On 01/05/2011 07:03 AM, Osier Yang wrote: -1 is a valid int, but not a valid cellno, so --cellno=-1 would now give a misleading message. I also don't like the fact that you are changing the semantics of vshCommandOptInt, but not the counterparts such as vshCommandOptUL. I'd rather see all the vshCommandOpt* functions have the same semantics regarding their found parameter. And, since some of the vshCommandOpt* return unsigned values, you can't rely on -1 as a sentinel return value to indicate argument present but invalid. You'd have to go with something different, such as altering the semantics of the found argument: instead of being a binary TRUE/FALSE return (using TRUE/FALSE and int* is nasty anyways, because we already have <stdbool.h> and could be using true/false and bool* instead), let's instead have it be a ternary value: found < 0 => argument was present, but invalid (not an integer); return value is 0 found == 0 => argument was missing; return value is 0 found > 0 => argument was present and valid; return value is its value but this means that all existing callers that pass NULL instead of &found as the third argument are silently ignoring errors, at which point, it seems like we should require a non-NULL found argument. But if we do that, we might as well go one step further, and swap the order of the API entirely (to force ALL callers to check for errors): int vshCommandOptUL(const vshCmd *cmd, const char *name, unsigned long *value) ATTRIBUTE_NONNULL(3); Return value is <0 on failure (*value untouched), 0 when option is absent (*value untouched), and >0 on success (*value updated). Swapping the API like that also has the benefit that a client can specify a non-zero default: unsigned long value = 1024; if (vshCommandOptUL(cmd, "arg", &value) < 0) { error; return FALSE; } use value rather than the current code usage of checking whether a value was supplied, and if not, supplying the default. Yes, an API swap like this would be a much bigger change, but it seems like it is cleaner in the long run (since invalid cellno can't be the only case where passing a non-integer string gets silently ignored back to the default integer value). Style nit: you used: if (cond) { abc; def; } else xyz; But we prefer either: if (!cond) xyz; else { abc; def; } or: if (cond) { abc; def; } else { xyz; } since HACKING documents that an else clause should only ever omit braces when the if clause also omitted braces, but an if clause can omit braces even when the else clause requires them. NACK as-is; but I agree that this is (at least one) real bug to be fixed. Does my idea for a broader rewrite of the vshCommandOpt* function semantics make sense? -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

* docs/hacking.html.in (Curly braces): Tighten recommendations to disallow if (cond) one-line; else { block; }. * HACKING: Regenerate. Suggested by Daniel P. Berrange. --- Fine by me - how does this look? HACKING | 35 ++++++++++++++++++++++------------- docs/hacking.html.in | 34 +++++++++++++++++++++++----------- 2 files changed, 45 insertions(+), 24 deletions(-) diff --git a/HACKING b/HACKING index d646709..4a71b37 100644 --- a/HACKING +++ b/HACKING @@ -161,33 +161,42 @@ Do this, instead: However, there is one exception in the other direction, when even a one-line block should have braces. That occurs when that one-line, brace-less block is -an "else" block, and the corresponding "then" block *does* use braces. In that -case, either put braces around the "else" block, or negate the "if"-condition -and swap the bodies, putting the one-line block first and making the longer, -multi-line block be the "else" block. +an "if" or "else" block, and the counterpart block *does* use braces. In that +case, put braces around both blocks. Also, if the "else" block is much shorter +than the "if" block, consider negating the "if"-condition and swapping the +bodies, putting the short block first and making the longer, multi-line block +be the "else" block. if (expr) { ... ... } else - x = y; // BAD: braceless "else" with braced "then" + x = y; // BAD: braceless "else" with braced "then", + // and short block last -This is preferred, especially when the multi-line body is more than a few -lines long, because it is easier to read and grasp the semantics of an -if-then-else block when the simpler block occurs first, rather than after the -more involved block: + if (expr) + x = y; // BAD: braceless "if" with braced "else" + else { + ... + ... + } - if (!expr) +Keeping braces consistent and putting the short block first is preferred, +especially when the multi-line body is more than a few lines long, because it +is easier to read and grasp the semantics of an if-then-else block when the +simpler block occurs first, rather than after the more involved block: + + if (!expr) { x = y; // putting the smaller block first is more readable - else { + } else { ... ... } -If you'd rather not negate the condition, then at least add braces: +But if negating a complex condition is too ugly, then at least add braces: - if (expr) { + if (complex expr not worth negating) { ... ... } else { diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 900e242..0d81b0b 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -209,11 +209,13 @@ <p> However, there is one exception in the other direction, when even a one-line block should have braces. That occurs when that one-line, - brace-less block is an <code>else</code> block, and the corresponding - <code>then</code> block <b>does</b> use braces. In that case, either - put braces around the <code>else</code> block, or negate the - <code>if</code>-condition and swap the bodies, putting the - one-line block first and making the longer, multi-line block be the + brace-less block is an <code>if</code> or <code>else</code> + block, and the counterpart block <b>does</b> use braces. In + that case, put braces around both blocks. Also, if + the <code>else</code> block is much shorter than + the <code>if</code> block, consider negating the + <code>if</code>-condition and swapping the bodies, putting the + short block first and making the longer, multi-line block be the <code>else</code> block. </p> @@ -223,31 +225,41 @@ ... } else - x = y; // BAD: braceless "else" with braced "then" + x = y; // BAD: braceless "else" with braced "then", + // and short block last + + if (expr) + x = y; // BAD: braceless "if" with braced "else" + else { + ... + ... + } </pre> <p> - This is preferred, especially when the multi-line body is more than a + Keeping braces consistent and putting the short block first is + preferred, especially when the multi-line body is more than a few lines long, because it is easier to read and grasp the semantics of an if-then-else block when the simpler block occurs first, rather than after the more involved block: </p> <pre> - if (!expr) + if (!expr) { x = y; // putting the smaller block first is more readable - else { + } else { ... ... } </pre> <p> - If you'd rather not negate the condition, then at least add braces: + But if negating a complex condition is too ugly, then at least + add braces: </p> <pre> - if (expr) { + if (complex expr not worth negating) { ... ... } else { -- 1.7.3.4