- Revision
- 207382
- Author
- dba...@webkit.org
- Date
- 2016-10-15 15:04:08 -0700 (Sat, 15 Oct 2016)
Log Message
prepare-ChangeLog erroneously said that a python __init__ method was deleted
https://bugs.webkit.org/show_bug.cgi?id=163456
Reviewed by Simon Fraser.
Fixes an issue where prepare-ChangeLog may list as deleted functions that are
immediately above added code.
Currently prepare-ChangeLog makes use of the same overlap detection algorithm
to compute the list of deleted functions as it does to compute added and modified
functions. We consider a function deleted if its entire function body and signature
are removed. It is sufficient to compare the list of functions before the patch
is applied and the list of functions are the patch is applied to identify
these functions.
* Scripts/prepare-ChangeLog: Fix some style nits, including using Camel Case for
variable names.
(actuallyGenerateFunctionLists): Modified to call computeModifiedFunctions(). Always
compute the list of functions in the file after the patch regardless of whether the
patch only contains deletions. We will compare this list against the list of functions
before the patch was applied to determine the deleted functions.
(computeModifiedFunctions): Renamed; formerly named generateFunctionListsByRanges.
Removed out argument for the seen functions as we no longer make use of when computing
the list of deleted functions.
(diffCommand): Update comment.
(generateFunctionListsByRanges): Deleted.
* Scripts/webkitperl/prepare-ChangeLog_unittest/generateFunctionLists.pl: Added more unit tests.
Modified Paths
Diff
Modified: trunk/Tools/ChangeLog (207381 => 207382)
--- trunk/Tools/ChangeLog 2016-10-15 21:52:09 UTC (rev 207381)
+++ trunk/Tools/ChangeLog 2016-10-15 22:04:08 UTC (rev 207382)
@@ -1,3 +1,33 @@
+2016-10-15 Daniel Bates <daba...@apple.com>
+
+ prepare-ChangeLog erroneously said that a python __init__ method was deleted
+ https://bugs.webkit.org/show_bug.cgi?id=163456
+
+ Reviewed by Simon Fraser.
+
+ Fixes an issue where prepare-ChangeLog may list as deleted functions that are
+ immediately above added code.
+
+ Currently prepare-ChangeLog makes use of the same overlap detection algorithm
+ to compute the list of deleted functions as it does to compute added and modified
+ functions. We consider a function deleted if its entire function body and signature
+ are removed. It is sufficient to compare the list of functions before the patch
+ is applied and the list of functions are the patch is applied to identify
+ these functions.
+
+ * Scripts/prepare-ChangeLog: Fix some style nits, including using Camel Case for
+ variable names.
+ (actuallyGenerateFunctionLists): Modified to call computeModifiedFunctions(). Always
+ compute the list of functions in the file after the patch regardless of whether the
+ patch only contains deletions. We will compare this list against the list of functions
+ before the patch was applied to determine the deleted functions.
+ (computeModifiedFunctions): Renamed; formerly named generateFunctionListsByRanges.
+ Removed out argument for the seen functions as we no longer make use of when computing
+ the list of deleted functions.
+ (diffCommand): Update comment.
+ (generateFunctionListsByRanges): Deleted.
+ * Scripts/webkitperl/prepare-ChangeLog_unittest/generateFunctionLists.pl: Added more unit tests.
+
2016-10-14 Simon Fraser <simon.fra...@apple.com>
REGRESSION (r206973): Running "webkit-patch suggest-reviewers" throws an AttributeError: 'NoneType' object has no attribute 'full_name'
Modified: trunk/Tools/Scripts/prepare-ChangeLog (207381 => 207382)
--- trunk/Tools/Scripts/prepare-ChangeLog 2016-10-15 21:52:09 UTC (rev 207381)
+++ trunk/Tools/Scripts/prepare-ChangeLog 2016-10-15 22:04:08 UTC (rev 207382)
@@ -69,6 +69,7 @@
sub changeLogDate($);
sub changeLogEmailAddressFromArgs($$);
sub changeLogNameFromArgs($$);
+sub computeModifiedFunctions($$$);
sub createPatchCommand($$$$);
sub decodeEntities($);
sub determinePropertyChanges($$$);
@@ -83,7 +84,6 @@
sub findOriginalFileFromSvn($);
sub generateFileList(\%$$$);
sub generateFunctionLists($$$$$);
-sub generateFunctionListsByRanges($$$$);
sub generateNewChangeLogs($$$$$$$$$$$$$$);
sub getLatestChangeLogs($);
sub get_function_line_ranges($$);
@@ -349,16 +349,20 @@
# and other code doesn't expect to see a trailing " character when sniffing a file extension.
chomp $file;
$file =~ s/ /\\ /g;
+
my %saw_function;
+ my @afterChangeFunctionRanges;
+
# Find all the functions in the file.
+ my $sourceFileHandle = $delegateHashRef->{openFile}($file);
+ next unless $sourceFileHandle;
+ my @afterChangeFunctionRanges = get_function_line_ranges($sourceFileHandle, $file);
+ close($sourceFileHandle);
+
+ # Find modified functions in the file.
if ($line_ranges_after_changed{$file}) {
- my $sourceFileHandle = $delegateHashRef->{openFile}($file);
- next unless $sourceFileHandle;
- my @function_ranges = get_function_line_ranges($sourceFileHandle, $file);
- close($sourceFileHandle);
-
my @change_ranges = (@{$line_ranges_after_changed{$file}}, []);
- my @functions = generateFunctionListsByRanges($file, \@change_ranges, \@function_ranges, \%saw_function);
+ my @functions = computeModifiedFunctions($file, \@change_ranges, \@afterChangeFunctionRanges);
# Format the list of functions.
if (@functions) {
@@ -370,12 +374,21 @@
if ($line_ranges_before_changed{$file}) {
my $originalFileHandle = $delegateHashRef->{openOriginalFile}($file, $gitCommit, $gitIndex, $mergeBase);
next unless $originalFileHandle;
- my @deleted_function_ranges = get_function_line_ranges($originalFileHandle, $file);
+ my @beforeChangeFunctionRanges = get_function_line_ranges($originalFileHandle, $file);
close($originalFileHandle);
- my @change_ranges = (@{$line_ranges_before_changed{$file}}, []);
- my @functions = generateFunctionListsByRanges($file, \@change_ranges, \@deleted_function_ranges, \%saw_function);
+ my %existsAfterChange = map { $_->[2] => 1 } @afterChangeFunctionRanges;
+ my @functions;
+ my %sawFunctions;
+ for my $functionRange (@beforeChangeFunctionRanges) {
+ my $functionName = $functionRange->[2];
+ if (!$existsAfterChange{$functionName} && !$sawFunctions{$functionName}) {
+ push @functions, $functionName;
+ $sawFunctions{$functionName} = 1;
+ }
+ }
+
# Format the list of deleted functions.
if (@functions) {
$functionLists->{$file} = "" if !defined $functionLists->{$file};
@@ -385,15 +398,17 @@
}
}
-sub generateFunctionListsByRanges($$$$)
+sub computeModifiedFunctions($$$)
{
- my ($file, $changed_line_ranges, $function_ranges, $saw_function) = @_;
+ my ($file, $changedLineRanges, $functionRanges) = @_;
+ my %sawFunction;
+
# Find all the modified functions.
my @functions;
- my @change_ranges = @{$changed_line_ranges};
+ my @change_ranges = @{$changedLineRanges};
my @change_range = (0, 0);
- FUNCTION: foreach my $function_range_ref (@{$function_ranges}) {
+ FUNCTION: foreach my $function_range_ref (@{$functionRanges}) {
my @function_range = @{$function_range_ref};
# FIXME: This is a hack. If the function name is empty, skip it.
@@ -414,8 +429,8 @@
# If an overlap with this function range, record the function name.
if ($change_range[1] >= $function_range[0]
and $change_range[0] <= $function_range[1]) {
- if (!$saw_function->{$function_range[2]}) {
- $saw_function->{$function_range[2]} = 1;
+ if (!$sawFunction{$function_range[2]}) {
+ $sawFunction{$function_range[2]} = 1;
push @functions, $function_range[2];
}
next FUNCTION;
@@ -1996,7 +2011,7 @@
{
my ($paths, $gitCommit, $gitIndex, $mergeBase) = @_;
- # The function overlap detection logic in generateFunctionListsByRanges() assumes that its line
+ # The function overlap detection logic in computeModifiedFunctions() assumes that its line
# ranges were from a unified diff without any context lines.
my $command;
if (isSVN()) {
Modified: trunk/Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/generateFunctionLists.pl (207381 => 207382)
--- trunk/Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/generateFunctionLists.pl 2016-10-15 21:52:09 UTC (rev 207381)
+++ trunk/Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/generateFunctionLists.pl 2016-10-15 22:04:08 UTC (rev 207382)
@@ -75,6 +75,20 @@
}
EOF
+my $EXAMPLE_JAVASCRIPT_WITH_SAME_FUNCTION_DEFINED_TWICE = <<EOF;
+function f() {
+ return 5;
+}
+
+function f() {
+ return 6;
+}
+
+function g() {
+ return 7;
+}
+EOF
+
my @testCaseHashRefs = (
###
# 0 lines of context
@@ -148,6 +162,131 @@
EOF
},
{
+ testName => "Unified diff with 0 context; add function immediately after another function",
+ inputText => $EXAMPLE_CPP,
+ diffToApply => <<EOF,
+diff --git a/fib.cpp b/fib.cpp
+--- a/fib.cpp
++++ b/fib.cpp
+@@ -11,0 +12,4 @@ unsigned fibPlusFive(unsigned n)
++unsigned fibPlusSeven(unsigned n)
++{
++ return fib(n) + 7;
++}
+EOF
+ expected => <<EOF
+
+ (fibPlusSeven):
+EOF
+},
+{
+ testName => "Unified diff with 0 context; add function at the end of the file",
+ inputText => $EXAMPLE_CPP,
+ diffToApply => <<EOF,
+diff --git a/fib.cpp b/fib.cpp
+--- a/fib.cpp
++++ b/fib.cpp
+@@ -39,0 +40,5 @@ int main(int argc, const char* argv[])
++
++unsigned fibPlusSeven(unsigned n)
++{
++ return fib(n) + 7;
++}
+EOF
+ expected => <<EOF
+
+ (fibPlusSeven):
+EOF
+},
+{
+ testName => "Unified diff with 0 context; rename function",
+ inputText => $EXAMPLE_CPP,
+ diffToApply => <<EOF,
+diff --git a/fib.cpp b/fib.cpp
+--- a/fib.cpp
++++ b/fib.cpp
+@@ -8 +8 @@ unsigned fib(unsigned);
+-unsigned fibPlusFive(unsigned n)
++unsigned fibPlusFive2(unsigned n)
+EOF
+ expected => <<EOF
+
+ (fibPlusFive2):
+ (fibPlusFive): Deleted.
+EOF
+},
+{
+ testName => "Unified diff with 0 context; replace function",
+ inputText => $EXAMPLE_CPP,
+ diffToApply => <<EOF,
+diff --git a/fib.cpp b/fib.cpp
+--- a/fib.cpp
++++ b/fib.cpp
+@@ -8,4 +8,5 @@ unsigned fib(unsigned);
+-unsigned fibPlusFive(unsigned n)
+-{
+- return fib(n) + 5;
+-}
++unsigned fibPlusSeven(unsigned n)
++{ // First comment
++ // Second comment
++ return fib(n) + 7;
++};
+EOF
+ expected => <<EOF
+
+ (fibPlusSeven):
+ (fibPlusFive): Deleted.
+EOF
+},
+{
+ testName => "Unified diff with 0 context; move function from the top of the file to the end of the file",
+ inputText => $EXAMPLE_CPP,
+ diffToApply => <<EOF,
+diff --git a/fib.cpp b/fib.cpp
+--- a/fib.cpp
++++ b/fib.cpp
+@@ -8,5 +7,0 @@ unsigned fib(unsigned);
+-unsigned fibPlusFive(unsigned n)
+-{
+- return fib(n) + 5;
+-}
+-
+@@ -39,0 +35,5 @@ int main(int argc, const char* argv[])
++
++unsigned fibPlusFive(unsigned n)
++{
++ return fib(n) + 5;
++}
+EOF
+ expected => <<EOF
+
+ (fibPlusFive):
+EOF
+},
+{
+ testName => "Unified diff with 0 context; remove functions with the same name",
+ inputText => $EXAMPLE_JAVASCRIPT_WITH_SAME_FUNCTION_DEFINED_TWICE,
+ diffToApply => <<EOF,
+diff --git a/fib.cpp b/fib.cpp
+--- a/fib.cpp
++++ b/fib.cpp
+@@ -1,8 +0,0 @@
+-function f() {
+- return 5;
+-}
+-
+-function f() {
+- return 6;
+-}
+-
+EOF
+ expected => <<EOF
+
+ (f): Deleted.
+EOF
+},
+{
# This is a contrived example and would cause a redefinition error in C++, but an analagous change in a _javascript_ script would be fine.
testName => "Unified diff with 0 context; add function above function with the same name",
inputText => $EXAMPLE_CPP,
