<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[207382] trunk/Tools</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/207382">207382</a></dd>
<dt>Author</dt> <dd>dbates@webkit.org</dd>
<dt>Date</dt> <dd>2016-10-15 15:04:08 -0700 (Sat, 15 Oct 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>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.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkToolsChangeLog">trunk/Tools/ChangeLog</a></li>
<li><a href="#trunkToolsScriptsprepareChangeLog">trunk/Tools/Scripts/prepare-ChangeLog</a></li>
<li><a href="#trunkToolsScriptswebkitperlprepareChangeLog_unittestgenerateFunctionListspl">trunk/Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/generateFunctionLists.pl</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkToolsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Tools/ChangeLog (207381 => 207382)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/ChangeLog 2016-10-15 21:52:09 UTC (rev 207381)
+++ trunk/Tools/ChangeLog 2016-10-15 22:04:08 UTC (rev 207382)
</span><span class="lines">@@ -1,3 +1,33 @@
</span><ins>+2016-10-15 Daniel Bates <dabates@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.
+
</ins><span class="cx"> 2016-10-14 Simon Fraser <simon.fraser@apple.com>
</span><span class="cx">
</span><span class="cx"> REGRESSION (r206973): Running "webkit-patch suggest-reviewers" throws an AttributeError: 'NoneType' object has no attribute 'full_name'
</span></span></pre></div>
<a id="trunkToolsScriptsprepareChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Tools/Scripts/prepare-ChangeLog (207381 => 207382)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -69,6 +69,7 @@
</span><span class="cx"> sub changeLogDate($);
</span><span class="cx"> sub changeLogEmailAddressFromArgs($$);
</span><span class="cx"> sub changeLogNameFromArgs($$);
</span><ins>+sub computeModifiedFunctions($$$);
</ins><span class="cx"> sub createPatchCommand($$$$);
</span><span class="cx"> sub decodeEntities($);
</span><span class="cx"> sub determinePropertyChanges($$$);
</span><span class="lines">@@ -83,7 +84,6 @@
</span><span class="cx"> sub findOriginalFileFromSvn($);
</span><span class="cx"> sub generateFileList(\%$$$);
</span><span class="cx"> sub generateFunctionLists($$$$$);
</span><del>-sub generateFunctionListsByRanges($$$$);
</del><span class="cx"> sub generateNewChangeLogs($$$$$$$$$$$$$$);
</span><span class="cx"> sub getLatestChangeLogs($);
</span><span class="cx"> sub get_function_line_ranges($$);
</span><span class="lines">@@ -349,16 +349,20 @@
</span><span class="cx"> # and other code doesn't expect to see a trailing " character when sniffing a file extension.
</span><span class="cx"> chomp $file;
</span><span class="cx"> $file =~ s/ /\\ /g;
</span><ins>+
</ins><span class="cx"> my %saw_function;
</span><ins>+ my @afterChangeFunctionRanges;
+
</ins><span class="cx"> # Find all the functions in the file.
</span><ins>+ my $sourceFileHandle = $delegateHashRef->{openFile}($file);
+ next unless $sourceFileHandle;
+ my @afterChangeFunctionRanges = get_function_line_ranges($sourceFileHandle, $file);
+ close($sourceFileHandle);
+
+ # Find modified functions in the file.
</ins><span class="cx"> if ($line_ranges_after_changed{$file}) {
</span><del>- my $sourceFileHandle = $delegateHashRef->{openFile}($file);
- next unless $sourceFileHandle;
- my @function_ranges = get_function_line_ranges($sourceFileHandle, $file);
- close($sourceFileHandle);
-
</del><span class="cx"> my @change_ranges = (@{$line_ranges_after_changed{$file}}, []);
</span><del>- my @functions = generateFunctionListsByRanges($file, \@change_ranges, \@function_ranges, \%saw_function);
</del><ins>+ my @functions = computeModifiedFunctions($file, \@change_ranges, \@afterChangeFunctionRanges);
</ins><span class="cx">
</span><span class="cx"> # Format the list of functions.
</span><span class="cx"> if (@functions) {
</span><span class="lines">@@ -370,12 +374,21 @@
</span><span class="cx"> if ($line_ranges_before_changed{$file}) {
</span><span class="cx"> my $originalFileHandle = $delegateHashRef->{openOriginalFile}($file, $gitCommit, $gitIndex, $mergeBase);
</span><span class="cx"> next unless $originalFileHandle;
</span><del>- my @deleted_function_ranges = get_function_line_ranges($originalFileHandle, $file);
</del><ins>+ my @beforeChangeFunctionRanges = get_function_line_ranges($originalFileHandle, $file);
</ins><span class="cx"> close($originalFileHandle);
</span><span class="cx">
</span><del>- my @change_ranges = (@{$line_ranges_before_changed{$file}}, []);
- my @functions = generateFunctionListsByRanges($file, \@change_ranges, \@deleted_function_ranges, \%saw_function);
</del><ins>+ my %existsAfterChange = map { $_->[2] => 1 } @afterChangeFunctionRanges;
</ins><span class="cx">
</span><ins>+ my @functions;
+ my %sawFunctions;
+ for my $functionRange (@beforeChangeFunctionRanges) {
+ my $functionName = $functionRange->[2];
+ if (!$existsAfterChange{$functionName} && !$sawFunctions{$functionName}) {
+ push @functions, $functionName;
+ $sawFunctions{$functionName} = 1;
+ }
+ }
+
</ins><span class="cx"> # Format the list of deleted functions.
</span><span class="cx"> if (@functions) {
</span><span class="cx"> $functionLists->{$file} = "" if !defined $functionLists->{$file};
</span><span class="lines">@@ -385,15 +398,17 @@
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span><del>-sub generateFunctionListsByRanges($$$$)
</del><ins>+sub computeModifiedFunctions($$$)
</ins><span class="cx"> {
</span><del>- my ($file, $changed_line_ranges, $function_ranges, $saw_function) = @_;
</del><ins>+ my ($file, $changedLineRanges, $functionRanges) = @_;
</ins><span class="cx">
</span><ins>+ my %sawFunction;
+
</ins><span class="cx"> # Find all the modified functions.
</span><span class="cx"> my @functions;
</span><del>- my @change_ranges = @{$changed_line_ranges};
</del><ins>+ my @change_ranges = @{$changedLineRanges};
</ins><span class="cx"> my @change_range = (0, 0);
</span><del>- FUNCTION: foreach my $function_range_ref (@{$function_ranges}) {
</del><ins>+ FUNCTION: foreach my $function_range_ref (@{$functionRanges}) {
</ins><span class="cx"> my @function_range = @{$function_range_ref};
</span><span class="cx">
</span><span class="cx"> # FIXME: This is a hack. If the function name is empty, skip it.
</span><span class="lines">@@ -414,8 +429,8 @@
</span><span class="cx"> # If an overlap with this function range, record the function name.
</span><span class="cx"> if ($change_range[1] >= $function_range[0]
</span><span class="cx"> and $change_range[0] <= $function_range[1]) {
</span><del>- if (!$saw_function->{$function_range[2]}) {
- $saw_function->{$function_range[2]} = 1;
</del><ins>+ if (!$sawFunction{$function_range[2]}) {
+ $sawFunction{$function_range[2]} = 1;
</ins><span class="cx"> push @functions, $function_range[2];
</span><span class="cx"> }
</span><span class="cx"> next FUNCTION;
</span><span class="lines">@@ -1996,7 +2011,7 @@
</span><span class="cx"> {
</span><span class="cx"> my ($paths, $gitCommit, $gitIndex, $mergeBase) = @_;
</span><span class="cx">
</span><del>- # The function overlap detection logic in generateFunctionListsByRanges() assumes that its line
</del><ins>+ # The function overlap detection logic in computeModifiedFunctions() assumes that its line
</ins><span class="cx"> # ranges were from a unified diff without any context lines.
</span><span class="cx"> my $command;
</span><span class="cx"> if (isSVN()) {
</span></span></pre></div>
<a id="trunkToolsScriptswebkitperlprepareChangeLog_unittestgenerateFunctionListspl"></a>
<div class="modfile"><h4>Modified: trunk/Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/generateFunctionLists.pl (207381 => 207382)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -75,6 +75,20 @@
</span><span class="cx"> }
</span><span class="cx"> EOF
</span><span class="cx">
</span><ins>+my $EXAMPLE_JAVASCRIPT_WITH_SAME_FUNCTION_DEFINED_TWICE = <<EOF;
+function f() {
+ return 5;
+}
+
+function f() {
+ return 6;
+}
+
+function g() {
+ return 7;
+}
+EOF
+
</ins><span class="cx"> my @testCaseHashRefs = (
</span><span class="cx"> ###
</span><span class="cx"> # 0 lines of context
</span><span class="lines">@@ -148,6 +162,131 @@
</span><span class="cx"> EOF
</span><span class="cx"> },
</span><span class="cx"> {
</span><ins>+ 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
+},
+{
</ins><span class="cx"> # This is a contrived example and would cause a redefinition error in C++, but an analagous change in a JavaScript script would be fine.
</span><span class="cx"> testName => "Unified diff with 0 context; add function above function with the same name",
</span><span class="cx"> inputText => $EXAMPLE_CPP,
</span></span></pre>
</div>
</div>
</body>
</html>