Re: [PATCH 05/18] Turn double-negated expressions into simple expressions

On Fri, Jun 7, 2013 at 1:04 PM, Célestin Matte
[off-list ref] wrote:

Le 07/06/2013 06:12, Eric Sunshine a écrit :

quoted

On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
[off-list ref] wrote:

quoted

                } elsif ($cmd[0] eq "import") {
-                       die("Invalid arguments for import\n") unless ($cmd[1] ne "" && !defined($cmd[2]));
+                       die("Invalid arguments for import\n") if ($cmd[1] eq "" || defined($cmd[2]));
                        mw_import($cmd[1]);
                } elsif ($cmd[0] eq "option") {
-                       die("Too many arguments for option\n") unless ($cmd[1] ne "" && $cmd[2] ne "" && !defined($cmd[3]));
+                       die("Too many arguments for option\n") if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));

Not new in this patch, but isn't this diagnostic misleading? It will
(falsely) claim "too many arguments" if $cmd[1] or $cmd[2] is an empty
string. Perhaps it should be reworded like the 'import' diagnostic and
say "Invalid arguments for option".

We could even be more precise and separate the cases, i.e., die("Too
many arguments") when too many arguments are defined and die("Invalid
arguments") when there are empty strings.
Not sure if I should integrate it in this patch, though.

If you do choose to be more precise, it should be done as a separate
patch. Each conceptually distinct change should have its own patch.
Doing so makes changes easier to review and (generally) easier to
cherry-pick. For example, in this particular case, "simplify
doubly-negated expressions" is quite conceptually distinct from "emit
more precise diagnostics". (Textually the changes may happen to
overlap, but conceptually they are unrelated.)