Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
On 01/24/11 18:47, Markus Armbruster wrote: Jes Sorensen jes.soren...@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
Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
Jes Sorensen jes.soren...@redhat.com writes: On 01/24/11 18:47, Markus Armbruster wrote: Jes Sorensen jes.soren...@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. [...]
Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
On 01/25/11 11:17, Markus Armbruster wrote: Jes Sorensen jes.soren...@redhat.com writes: On 01/24/11 18:47, Markus Armbruster wrote: Jes Sorensen jes.soren...@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
[Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@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:
--
1.7.3.4
Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
On 01/24/11 17:08, Markus Armbruster wrote:
jes.soren...@redhat.com writes:
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@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
Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
jes.soren...@redhat.com writes:
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@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.
Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
Jes Sorensen jes.soren...@redhat.com writes:
On 01/24/11 17:08, Markus Armbruster wrote:
jes.soren...@redhat.com writes:
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@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.
Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
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
Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
Jes Sorensen jes.soren...@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.
