Modify webkitdirs do reuse checkForArgumentAndRemoveFromARGV
Created attachment 217307 [details] Patch
Created attachment 217308 [details] Patch
Comment on attachment 217308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217308&action=review > Tools/ChangeLog:9 > + functions instead of reusing them as it's being done by all other functions. s/as it's/as is/, I think? > Tools/ChangeLog:17 > + (checkForArgumentAndRemoveFromArrayRef): Fixes a bug in elements removal. what kind of bug! more explanation would be nice :) > Tools/Scripts/webkitdirs.pm:846 > +sub checkForArgumentAndRemoveFromARGVGettingValue($$) maybe getArgumentAndRemoveFromARGV? 'get' prefix is closer to 'checkFor'. I dunno. > Tools/Scripts/webkitdirs.pm:850 > + return 0 unless ($#matchingIndices != 1); why 0 instead of undef or whatever?
(In reply to comment #3) > (From update of attachment 217308 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217308&action=review Thanks for the review. > > > Tools/ChangeLog:9 > > + functions instead of reusing them as it's being done by all other functions. > > s/as it's/as is/, I think? ok > > > Tools/ChangeLog:17 > > + (checkForArgumentAndRemoveFromArrayRef): Fixes a bug in elements removal. > > what kind of bug! more explanation would be nice :) > ok > > Tools/Scripts/webkitdirs.pm:846 > > +sub checkForArgumentAndRemoveFromARGVGettingValue($$) > > maybe getArgumentAndRemoveFromARGV? 'get' prefix is closer to 'checkFor'. I dunno. > I've just followed the pattern the other functions were already using > > Tools/Scripts/webkitdirs.pm:850 > > + return 0 unless ($#matchingIndices != 1); > > why 0 instead of undef or whatever? In this case, the return value is checked in a boolean expression, so '0' or 'undef' has the same effect (in perl)
Created attachment 217313 [details] Patch
Comment on attachment 217313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217313&action=review This patch looks good. I'm r-'ing this patch because of the lack of a test. > Tools/Scripts/webkitdirs.pm:393 > + if (checkForArgumentAndRemoveFromARGVGettingValue('--sdk', \$sdk)) { Nit: ' => " That is, we use double quotes for string literals unless we explicitly don't want string interpolation. > Tools/Scripts/webkitdirs.pm:395 > + } elsif (checkForArgumentAndRemoveFromARGV('--device')) { Ditto. > Tools/Scripts/webkitdirs.pm:397 > + } elsif (checkForArgumentAndRemoveFromARGV('--sim') || Ditto. > Tools/Scripts/webkitdirs.pm:398 > + checkForArgumentAndRemoveFromARGV('--simulator')) { Ditto. > Tools/Scripts/webkitdirs.pm:614 > + if (checkForArgumentAndRemoveFromARGV('--debug')) { Ditto. > Tools/Scripts/webkitdirs.pm:616 > + } elsif(checkForArgumentAndRemoveFromARGV('--release')) { Ditto. > Tools/Scripts/webkitdirs.pm:618 > + } elsif (checkForArgumentAndRemoveFromARGV('--profile') || checkForArgumentAndRemoveFromARGV('--profiling')) { Ditto. > Tools/Scripts/webkitdirs.pm:653 > + if (checkForArgumentAndRemoveFromARGV('--32-bit')) { Ditto. > Tools/Scripts/webkitdirs.pm:881 > + my $removeOffset = 0; > foreach my $index (@indicesToRemove) { > - splice(@$arrayRef, $index, 1); > + splice(@$arrayRef, $index - $removeOffset++, 1); Can we write a test for this change? You may find it beneficial to look at the tests in <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/> when writing such a test, including <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/VCSUtils_unittest/removeEOL.pl>.
(In reply to comment #6) > (From update of attachment 217313 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217313&action=review > > This patch looks good. I'm r-'ing this patch because of the lack of a test. > > > Tools/Scripts/webkitdirs.pm:393 > > + if (checkForArgumentAndRemoveFromARGVGettingValue('--sdk', \$sdk)) { > > Nit: ' => " > > That is, we use double quotes for string literals unless we explicitly don't want string interpolation. > > > Tools/Scripts/webkitdirs.pm:395 > > + } elsif (checkForArgumentAndRemoveFromARGV('--device')) { > > Ditto. > > > Tools/Scripts/webkitdirs.pm:397 > > + } elsif (checkForArgumentAndRemoveFromARGV('--sim') || > > Ditto. > > > Tools/Scripts/webkitdirs.pm:398 > > + checkForArgumentAndRemoveFromARGV('--simulator')) { > > Ditto. > > > Tools/Scripts/webkitdirs.pm:614 > > + if (checkForArgumentAndRemoveFromARGV('--debug')) { > > Ditto. > > > Tools/Scripts/webkitdirs.pm:616 > > + } elsif(checkForArgumentAndRemoveFromARGV('--release')) { > > Ditto. > > > Tools/Scripts/webkitdirs.pm:618 > > + } elsif (checkForArgumentAndRemoveFromARGV('--profile') || checkForArgumentAndRemoveFromARGV('--profiling')) { > > Ditto. > > > Tools/Scripts/webkitdirs.pm:653 > > + if (checkForArgumentAndRemoveFromARGV('--32-bit')) { > > Ditto. > Ok for all above. > > Tools/Scripts/webkitdirs.pm:881 > > + my $removeOffset = 0; > > foreach my $index (@indicesToRemove) { > > - splice(@$arrayRef, $index, 1); > > + splice(@$arrayRef, $index - $removeOffset++, 1); > > Can we write a test for this change? > > You may find it beneficial to look at the tests in <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/> when writing such a test, including <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/VCSUtils_unittest/removeEOL.pl>. Ok, I'll add some unit test cases for it :) thanks
Created attachment 217337 [details] Patch
Comment on attachment 217337 [details] Patch 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/checkForArgumentAndRemoveFromArrayRef.pl:1 > +#!/usr/bin/perl Please add the -w argument to enable warning diagnostics: #!/usr/bin/perl -w > Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.pl:26 > +use strict; Please include the warnings module: use warnings; > Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.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_unittest/removeEOL.pl> as an example. > Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.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.
Created attachment 217476 [details] Patch
(In reply to comment #8) > Created an attachment (id=217337) [details] > Patch (In reply to comment #9) > (From update of attachment 217337 [details]) > 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. > Just sent a new patch only without the bug fix and filed a new bug (https://bugs.webkit.org/show_bug.cgi?id=124676) to address the bug separately as you suggested. > > Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.pl:1 > > +#!/usr/bin/perl > > Please add the -w argument to enable warning diagnostics: #!/usr/bin/perl -w > > > Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.pl:26 > > +use strict; > > Please include the warnings module: use warnings; > > > Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.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_unittest/removeEOL.pl> as an example. > > > Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.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. Sounds good, I'll do that. thanks
Comment on attachment 217476 [details] Patch Thank you Nick for iterating on this patch.
Comment on attachment 217476 [details] Patch Clearing flags on attachment: 217476 Committed r159599: <http://trac.webkit.org/changeset/159599>
All reviewed patches have been landed. Closing bug.
After this patch, I'm getting errors like the one below everywhere: server:git-webkit rniwa$ Tools/Scripts/run-webkit-tests --debug LayoutTests/fast/dom LayoutTests/fast/html LayoutTests/fast/forms LayoutTests/svg LayoutTests/html5lib/ Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851. Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852. Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851. Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852. Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851. Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852. Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851. Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852. Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851. Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852. Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851. Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852. We're also seeing them on various builders on build.webkit.org. Please fix it ASAP.
(In reply to comment #15) > After this patch, I'm getting errors like the one below everywhere: > server:git-webkit rniwa$ Tools/Scripts/run-webkit-tests --debug LayoutTests/fast/dom LayoutTests/fast/html LayoutTests/fast/forms LayoutTests/svg LayoutTests/html5lib/ > Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851. > Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852. > Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851. > Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852. > Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851. > Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852. > Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851. > Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852. > Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851. > Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852. > Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851. > Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852. > > We're also seeing them on various builders on build.webkit.org. > > Please fix it ASAP. fixed in https://trac.webkit.org/changeset/159616