On Fri, Jun 7, 2013 at 1:04 PM, Célestin Matte
<celestin.ma...@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.ma...@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.)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
