[webkit-reviews] review denied: [Bug 124581] Modify webkitdirs to reuse checkForArgumentAndRemoveFromARGV : [Attachment 217337] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 20 13:20:39 PST 2013
Daniel Bates <dbates at webkit.org> has denied Nick Diego Yamane (diegoyam)
<nick.yamane at openbossa.org>'s request for review:
Bug 124581: Modify webkitdirs to reuse checkForArgumentAndRemoveFromARGV
https://bugs.webkit.org/show_bug.cgi?id=124581
Attachment 217337: Patch
https://bugs.webkit.org/attachment.cgi?id=217337&action=review
------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=217337&action=review
This patch is close. I'm r-'ing this patch since we should extract the bug fix
into a separate bug and patch. See my reasoning below for more details.
> Tools/ChangeLog:20
> + (checkForArgumentAndRemoveFromArrayRef): Bugfix: It was removing
wrong
> + elements when there were more then one occurrence of that argument.
> + E.g: Checking for 'a' in {a, b, a, c}, the resulting array would be
> + {b, a}, when it should be {b, c}.
Please extract this fix into a separate bug and patch. In general, a patch
should represent a single bug fix. The advantage of this approach is that it
makes it straightforward to track down a regression and/or revert a change set
without inadvertently blaming or reverting code that wasn't the cause of a
regression.
>
Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArray
Ref.pl:1
> +#!/usr/bin/perl
Please add the -w argument to enable warning diagnostics: #!/usr/bin/perl -w
>
Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArray
Ref.pl:26
> +use strict;
Please include the warnings module: use warnings;
>
Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArray
Ref.pl:27
> +use Test::Simple tests => 4;
I suggest we use module Test::More instead of Test::Simple because it exposes
additional assertion routines, such as is_deeply(). The assertion routine
is_deeply() can compare complex structures, including arrays, and print where
the structures differ (if they do). Then we don't need to implement a custom
array equality function. See
<http://search.cpan.org/~rjbs/Test-Simple-1.001002/lib/Test/More.pm> for more
details.
I didn't mean to imply that you should use Test::Simple when I suggested
looking at the test
<http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/VCSUtils_unittes
t/removeEOL.pl> as an example.
>
Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArray
Ref.pl:47
> +sub equalArrays(\@\@)
> +{
> + my @array1 = @{ shift() };
> + my @array2 = @{ shift() };
> + return 0 if ($#array1 != $#array2);
> +
> + for(my $i = 0; $i <= $#array1; ++$i) {
> + if ($array1[$i] ne $array2[$i]) {
> + return 0;
> + }
> + }
> + return 1;
> +}
As aforementioned above, I suggest we use Test::More::is_deeply() instead of
implemented our own array equality function as the former provides fails with
verbose output when there is a difference between the specified arrays.
More information about the webkit-reviews
mailing list