Patchwork [4/4] strtosz(): Use suffix macros in switch() statement

login
register
mail settings
Submitter Jes Sorensen
Date Jan. 24, 2011, 3:33 p.m.
Message ID <1295883211-18288-5-git-send-email-Jes.Sorensen@redhat.com>
Download mbox | patch
Permalink /patch/80185/
State New
Headers show

Comments

Jes Sorensen - Jan. 24, 2011, 3:33 p.m.
From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 cutils.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
Markus Armbruster - Jan. 24, 2011, 4:08 p.m.
Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  cutils.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 369a016..8d562b2 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -324,26 +324,26 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
>          }
>      }
>      switch (qemu_toupper(d)) {
> -    case 'B':
> +    case STRTOSZ_DEFSUFFIX_B:
>          mul = 1;
>          if (mul_required) {
>              goto fail;
>          }
>          break;
> -    case 'K':
> +    case STRTOSZ_DEFSUFFIX_KB:
>          mul = 1 << 10;
>          break;
>      case 0:
>          if (mul_required) {
>              goto fail;
>          }
> -    case 'M':
> +    case STRTOSZ_DEFSUFFIX_MB:
>          mul = 1ULL << 20;
>          break;
> -    case 'G':
> +    case STRTOSZ_DEFSUFFIX_GB:
>          mul = 1ULL << 30;
>          break;
> -    case 'T':
> +    case STRTOSZ_DEFSUFFIX_TB:
>          mul = 1ULL << 40;
>          break;
>      default:

Phony abstraction.  And it leaks: code here assumes the
STRTOSZ_DEFSUFFIX_T* are all upper case.
Jes Sorensen - Jan. 24, 2011, 4:10 p.m.
On 01/24/11 17:08, Markus Armbruster wrote:
> Jes.Sorensen@redhat.com writes:
> 
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  cutils.c |   10 +++++-----
>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/cutils.c b/cutils.c
>> index 369a016..8d562b2 100644
>> --- a/cutils.c
>> +++ b/cutils.c
>> @@ -324,26 +324,26 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
>>          }
>>      }
>>      switch (qemu_toupper(d)) {
>> -    case 'B':
>> +    case STRTOSZ_DEFSUFFIX_B:
>>          mul = 1;
>>          if (mul_required) {
>>              goto fail;
>>          }
>>          break;
>> -    case 'K':
>> +    case STRTOSZ_DEFSUFFIX_KB:
>>          mul = 1 << 10;
>>          break;
>>      case 0:
>>          if (mul_required) {
>>              goto fail;
>>          }
>> -    case 'M':
>> +    case STRTOSZ_DEFSUFFIX_MB:
>>          mul = 1ULL << 20;
>>          break;
>> -    case 'G':
>> +    case STRTOSZ_DEFSUFFIX_GB:
>>          mul = 1ULL << 30;
>>          break;
>> -    case 'T':
>> +    case STRTOSZ_DEFSUFFIX_TB:
>>          mul = 1ULL << 40;
>>          break;
>>      default:
> 
> Phony abstraction.  And it leaks: code here assumes the
> STRTOSZ_DEFSUFFIX_T* are all upper case.

qemu_toupper() - whats the problem?

Jes
Markus Armbruster - Jan. 24, 2011, 4:39 p.m.
Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> On 01/24/11 17:08, Markus Armbruster wrote:
>> Jes.Sorensen@redhat.com writes:
>> 
>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>
>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>> ---
>>>  cutils.c |   10 +++++-----
>>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/cutils.c b/cutils.c
>>> index 369a016..8d562b2 100644
>>> --- a/cutils.c
>>> +++ b/cutils.c
>>> @@ -324,26 +324,26 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
>>>          }
>>>      }
>>>      switch (qemu_toupper(d)) {
>>> -    case 'B':
>>> +    case STRTOSZ_DEFSUFFIX_B:
>>>          mul = 1;
>>>          if (mul_required) {
>>>              goto fail;
>>>          }
>>>          break;
>>> -    case 'K':
>>> +    case STRTOSZ_DEFSUFFIX_KB:
>>>          mul = 1 << 10;
>>>          break;
>>>      case 0:
>>>          if (mul_required) {
>>>              goto fail;
>>>          }
>>> -    case 'M':
>>> +    case STRTOSZ_DEFSUFFIX_MB:
>>>          mul = 1ULL << 20;
>>>          break;
>>> -    case 'G':
>>> +    case STRTOSZ_DEFSUFFIX_GB:
>>>          mul = 1ULL << 30;
>>>          break;
>>> -    case 'T':
>>> +    case STRTOSZ_DEFSUFFIX_TB:
>>>          mul = 1ULL << 40;
>>>          break;
>>>      default:
>> 
>> Phony abstraction.  And it leaks: code here assumes the
>> STRTOSZ_DEFSUFFIX_T* are all upper case.
>
> qemu_toupper() - whats the problem?

If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case
will not match any input.
Jes Sorensen - Jan. 24, 2011, 4:41 p.m.
On 01/24/11 17:39, Markus Armbruster wrote:
>>>> +    case STRTOSZ_DEFSUFFIX_TB:
>>>> >>>          mul = 1ULL << 40;
>>>> >>>          break;
>>>> >>>      default:
>>> >> 
>>> >> Phony abstraction.  And it leaks: code here assumes the
>>> >> STRTOSZ_DEFSUFFIX_T* are all upper case.
>> >
>> > qemu_toupper() - whats the problem?
> If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case
> will not match any input.

Right, so one has to be careful when adding new suffix constants.
However given that we already have all the likely to be used ones for
the near future, that isn't exactly a big issue.

On the other hand forcing the use of the macros makes it less likely
that someone specifies an unsupported constant by hitting 'y' instead of
't' or similar.

Jes
Markus Armbruster - Jan. 24, 2011, 5:47 p.m.
Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> On 01/24/11 17:39, Markus Armbruster wrote:
>>>>> +    case STRTOSZ_DEFSUFFIX_TB:
>>>>> >>>          mul = 1ULL << 40;
>>>>> >>>          break;
>>>>> >>>      default:
>>>> >> 
>>>> >> Phony abstraction.  And it leaks: code here assumes the
>>>> >> STRTOSZ_DEFSUFFIX_T* are all upper case.
>>> >
>>> > qemu_toupper() - whats the problem?
>> If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case
>> will not match any input.
>
> Right, so one has to be careful when adding new suffix constants.

Calls for a comment right next to the definition of the
STRTOSZ_DEFSUFFIX_T*.

I hate unstated restrictions that are hidden far away from the place
where you can break them.

> However given that we already have all the likely to be used ones for
> the near future, that isn't exactly a big issue.
>
> On the other hand forcing the use of the macros makes it less likely
> that someone specifies an unsupported constant by hitting 'y' instead of
> 't' or similar.

Takes a combination of butterfingers, cross-eyedness, and near-total
incompetence at basic smoke-testing.
Jes Sorensen - Jan. 25, 2011, 9:19 a.m.
On 01/24/11 18:47, Markus Armbruster wrote:
> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>>>>> qemu_toupper() - whats the problem?
>>> If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case
>>> will not match any input.
>>
>> Right, so one has to be careful when adding new suffix constants.
> 
> Calls for a comment right next to the definition of the
> STRTOSZ_DEFSUFFIX_T*.
> 
> I hate unstated restrictions that are hidden far away from the place
> where you can break them.

Well I am fine with a comment in the code.

>> However given that we already have all the likely to be used ones for
>> the near future, that isn't exactly a big issue.
>>
>> On the other hand forcing the use of the macros makes it less likely
>> that someone specifies an unsupported constant by hitting 'y' instead of
>> 't' or similar.
> 
> Takes a combination of butterfingers, cross-eyedness, and near-total
> incompetence at basic smoke-testing.

Not really, all it takes is someone writing a piece of code, not
thinking about it, therefore only testing things where a suffix is
specified as an argument and it gets missed.

Jes
Markus Armbruster - Jan. 25, 2011, 10:17 a.m.
Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> On 01/24/11 18:47, Markus Armbruster wrote:
>> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>>>>>> qemu_toupper() - whats the problem?
>>>> If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case
>>>> will not match any input.
>>>
>>> Right, so one has to be careful when adding new suffix constants.
>> 
>> Calls for a comment right next to the definition of the
>> STRTOSZ_DEFSUFFIX_T*.
>> 
>> I hate unstated restrictions that are hidden far away from the place
>> where you can break them.
>
> Well I am fine with a comment in the code.

Such a comment improves it from "wrong" to merely "ugly".  I can live
with that.

Thanks.

[...]
Jes Sorensen - Jan. 25, 2011, 11:54 a.m.
On 01/25/11 11:17, Markus Armbruster wrote:
> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
> 
>> On 01/24/11 18:47, Markus Armbruster wrote:
>>> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>>>>>>> qemu_toupper() - whats the problem?
>>>>> If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case
>>>>> will not match any input.
>>>>
>>>> Right, so one has to be careful when adding new suffix constants.
>>>
>>> Calls for a comment right next to the definition of the
>>> STRTOSZ_DEFSUFFIX_T*.
>>>
>>> I hate unstated restrictions that are hidden far away from the place
>>> where you can break them.
>>
>> Well I am fine with a comment in the code.
> 
> Such a comment improves it from "wrong" to merely "ugly".  I can live
> with that.

I realize that you view it as such, in other eyes it goes from hackish
to correct ... there is zero point in arguing over this, we can just
agree to disagree.

Jes

Patch

diff --git a/cutils.c b/cutils.c
index 369a016..8d562b2 100644
--- a/cutils.c
+++ b/cutils.c
@@ -324,26 +324,26 @@  int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
         }
     }
     switch (qemu_toupper(d)) {
-    case 'B':
+    case STRTOSZ_DEFSUFFIX_B:
         mul = 1;
         if (mul_required) {
             goto fail;
         }
         break;
-    case 'K':
+    case STRTOSZ_DEFSUFFIX_KB:
         mul = 1 << 10;
         break;
     case 0:
         if (mul_required) {
             goto fail;
         }
-    case 'M':
+    case STRTOSZ_DEFSUFFIX_MB:
         mul = 1ULL << 20;
         break;
-    case 'G':
+    case STRTOSZ_DEFSUFFIX_GB:
         mul = 1ULL << 30;
         break;
-    case 'T':
+    case STRTOSZ_DEFSUFFIX_TB:
         mul = 1ULL << 40;
         break;
     default: