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

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Jun 7, 2013 at 1:04 PM, Célestin Matte
<celestin.matte@ensimag.fr> wrote:
> Le 07/06/2013 06:12, Eric Sunshine a écrit :
>> On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
>> <celestin.matte@ensimag.fr> wrote:
>>>                 } 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.)