| Message ID | 1295883211-18288-5-git-send-email-Jes.Sorensen@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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
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.
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
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. [...]
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
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: