[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