The empty (WebCore): line seems to have fallen out of favor with reviewers: https://bugs.webkit.org/show_bug.cgi?id=104207#c18 https://bugs.webkit.org/show_bug.cgi?id=111149#c9 https://bugs.webkit.org/show_bug.cgi?id=112081#c12 We should change the prepareChangelog to not generate this line in the first place.
*** Bug 116439 has been marked as a duplicate of this bug. ***
prepare-ChangeLog is supposed to list function names. Not namespace and class names.
Created attachment 202632 [details] Patch
Comment on attachment 202632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202632&action=review > Tools/Scripts/prepare-ChangeLog:947 > + if (!(my ($matched) = grep $_ eq $function_name, @$namespaces)) This line is one of those wonderful unparseable-by-a-human lines of perl. Can we split it out into a few so I can tell what's going on? > Tools/Scripts/prepare-ChangeLog:948 > + { Brace goes on previous line.
Comment on attachment 202632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202632&action=review > Tools/ChangeLog:11 > + (get_function_line_ranges_for_cpp): > + Updated to return ranges of functions (excluding > + namespaces, classes, and structs) in a C++ file. "Updated to..." should appear on the right of : followed by a single space. It also seems like the line wrap should appear much later in the line. > Tools/Scripts/prepare-ChangeLog:933 > - return @ranges; > + my @new_ranges = delete_namespaces_from_ranges_for_cpp(@ranges, @all_namespaces); > + return @new_ranges; Why not "return delete_namespaces_from_ranges_for_cpp(@ranges, @all_namespaces);"? > Tools/Scripts/prepare-ChangeLog:935 > } > It appears the convention in this file is to have at least two blank lines between the curly brackets that closes the previous function and a comment line for the subsequent function. > Tools/Scripts/prepare-ChangeLog:939 > +# list of line ranges with the namespaces removed. > +sub delete_namespaces_from_ranges_for_cpp(\@\@) It seems like the convention in this file is to have a blank line between the last line of the comment and the first line of the corresponding function definition.
Comment on attachment 202632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202632&action=review >> Tools/Scripts/prepare-ChangeLog:947 >> + if (!(my ($matched) = grep $_ eq $function_name, @$namespaces)) > > This line is one of those wonderful unparseable-by-a-human lines of perl. Can we split it out into a few so I can tell what's going on? Yeah, we should declare $matched outside of the if statement. Also, if we're already using grep, why don't we just let grep generate new_ranges for us? e.g. http://stackoverflow.com/questions/3960028/how-can-i-filter-an-array-without-using-a-loop-in-perl
Comment on attachment 202632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202632&action=review > Tools/ChangeLog:8 > + * Scripts/prepare-ChangeLog: We have tests for this script in Tools/Scritps/webkitperl/prepare-ChangeLog_unittest. Did you run test-webkitperl to see if they still pass or not? We should be adding some tests at least. r- due to the lack of tests.
The proposed patch would not generate namespace-only, class-name-only, or struct-name-only lines in C++ files. Thus, if a change is made within a namespace/class/struct yet outside a function, it will not be reflected in the ChangeLog entry with a specific function name. Consider the sample file Source/foo/bar/sample.cpp below: 1 class Foo { 2 int x; 3 int y; 4 void bar() { 5 int z; 6 } 7 } ; Changing lines 2-3 would result in a ChangeLog entry with the following changes denoted: * Source/foo/bar/sample.cpp: Notice that it will not report a change in the class Foo, i.e. (Foo) will not be included in the ChangeLog. In contrast, changing line 5, which resides within the function bar() would result in an entry of the form: *Source/foo/bar/sample.cpp: (Foo::bar): Due to the nature of this patch, it causes the unit tests used in Tools/Scripts/test-webkitperl to fail when running Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl at line 83. This is because Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests-expected.txt considers (Class1) as a function on lines 158-159, while the patch does not (because it's not wrapped in a function).
Created attachment 203111 [details] Patch
Comment on attachment 203111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203111&action=review > Tools/ChangeLog:10 > + (get_function_line_ranges_for_cpp): Updated to return ranges of functions > + (excluding namespaces, classes, and structs) in a C++ file. I don't think this reflect the nature or semantics of this change. It's probably better to say something along the line of "Updated to ignore changes outside of functions". > Tools/ChangeLog:12 > + (delete_namespaces_from_ranges_for_cpp): > + (does_namespaces_contain_function): Say "Added.".
Comment on attachment 203111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203111&action=review > Tools/ChangeLog:7 > + You should summarize how this patch affects the way change log entries are generated as done in your comment #6.
Created attachment 203212 [details] Patch
<rdar://problem/14013385>
Comment on attachment 203212 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203212&action=review > Tools/ChangeLog:13 > + Reviewed by NOBODY (OOPS!). > + Reviewed by line should appear above the long description but below the bug url separated both by a single blank line as done here. > Tools/Scripts/prepare-ChangeLog:948 > +sub does_namespaces_contain_function(\@$) This function should probably be named either "do_namespaces_contain_function" or "is_function_in_namespaces".
Is it not possible to omit these these namespace/struct/class entries from the @ranges array when we build it?
It's possible, but I thought it'd be cleaner to remove them from @ranges after it's been composed because items are added to @ranges in 3 different locations in get_function_line_ranges_for_cpp.
Comment on attachment 203212 [details] Patch Attachment 203212 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/739604 New failing tests: fast/dom/location-new-window-no-crash.html
Created attachment 203261 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
(In reply to comment #17) > (From update of attachment 203212 [details]) > Attachment 203212 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/739604 > > New failing tests: > fast/dom/location-new-window-no-crash.html This is a lie, ignore it.
Created attachment 203263 [details] Patch
(In reply to comment #16) > It's possible, but I thought it'd be cleaner to remove them from @ranges after it's been composed because items are added to @ranges in 3 different locations in get_function_line_ranges_for_cpp. Can you elaborate? I take it wouldn't be sufficient to only insert into @ranges when we determine the closing brace for a function body?
Comment on attachment 203263 [details] Patch Attachment 203263 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/679169 New failing tests: fast/dom/location-new-window-no-crash.html
Created attachment 203272 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Sure, prepare-ChangeLog lines 812, 846, 872 are where function names are added to @ranges (search "push @ranges"). Each line handles a different case, i.e. when there's an open brace, close brace, etc. Your suggestion would require checking whether or not the function name is a namespace/class/struct name at each one of those lines. Implementing this patch by adding an additional function to remove namespaces/class/struct names from @ranges makes it easier for changes in the future, when anyone wanted to change the rules for what's tracked in a ChangeLog entry (i.e. they want all changes in a namespace to be logged, not just those within functions). The additional function does add a bit of overhead, but it shouldn't be much, especially since the number of namespace/class/struct names in a file is probably pretty small. To be honest, I'm not sure why we don't just use the closing brace for a function body.
(In reply to comment #24) > [...] > Implementing this patch by adding an additional function to remove namespaces/class/struct names from @ranges makes it easier for changes in the future, when anyone wanted to change the rules for what's tracked in a ChangeLog entry (i.e. they want all changes in a namespace to be logged, not just those within functions). The additional function does add a bit of overhead, but it shouldn't be much, especially since the number of namespace/class/struct names in a file is probably pretty small. > OK. That's fine. I was under the impression that we could remove the logic for inserting namespaces into @ranges such that get_function_line_ranges_for_cpp() only adds function names to @ranges. Should we revisit the decision to omit the listing of namespace/class/struct names and decide that we want to list the names of namespaces, structs, classes or some combination of those then we could restore the code before this change (since it's in SVN).
Comment on attachment 203263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203263&action=review > Tools/ChangeLog:9 > + The patch would not generate namespace-only, class-name-only, or struct-name-only > + lines in C++ files. Thus, if a change is made within a namespace/class/struct yet The first sentence doesn't read well. Maybe something like: Teach prepare-Changelog to not list the names of modified namespaces, classes, or structures. > Tools/ChangeLog:11 > + outside a function, it will not be reflected in the ChangeLog entry with a > + specific function name. See https://bugs.webkit.org/show_bug.cgi?id=113221#c8 The phrase "specific function name" is disingenuous since such entires aren't function names; they are the names of namespaces, classes, and structs. I suggest omitting the phrase "with a specific function name" such that the sentence reads: Thus, if a change is made within a namespace/class/struct yet outside a function, it will not be reflected in the ChangeLog entry. Event better, we should consider rewriting this sentence to talk about what prepare-Changelog will do after this change as opposed to talking about what prepare-Changelog will not do after this change. > Tools/ChangeLog:17 > + (get_function_line_ranges_for_cpp): Updated to ignore changes > + outside of functions. > + (excluding namespaces, classes, and structs) in a C++ file. When I first read this I interpreted it as saying that we will be ignoring changes outside of functions except when such changes occur within a namespace, class, or struct. Instead, the modifications to get_function_line_ranges_for_cpp() are so that we ignore changes within a namespace, class, or struct that aren't to a function. Regardless, such a sentence seems like a restatement of the change description (above). I suggest either omitting this sentence or modify it so that it reads well. > Tools/Scripts/prepare-ChangeLog:933 > + return delete_namespaces_from_ranges_for_cpp(@ranges, @all_namespaces); As aforementioned in comment 15, I wish we could just omit adding the name of namespaces, classes, or structs to @ranges instead of fixing @ranges up afterwards. > Tools/Scripts/prepare-ChangeLog:938 > +# and an array of namespaces declared in that file and return an updated Nit: return => returns > Tools/Scripts/prepare-ChangeLog:944 > + return grep {!is_function_in_namespace(@$namespaces, $$_[2])} @$ranges; It's unnecessary to dereference $namespaces since is_function_in_namespace() wants to work with a reference. It's sufficient to write $namespaces instead of @$namespaces and then modify the prototype of is_function_in_namespace to accept two scalars (e.g. sub is_function_in_namespace(\$$)). > Tools/Scripts/prepare-ChangeLog:951 > + return (grep $_ eq $function_name, @$namespaces); The notation used for grep() on this line differs from the notation used for grep() in delete_namespaces_from_ranges_for_cpp(). I suggest we pick a notation and stick with it for consistency.
(In reply to comment #26) > [..] > It's sufficient to write $namespaces instead of @$namespaces and then modify the prototype of is_function_in_namespace to accept two scalars (e.g. sub is_function_in_namespace(\$$)). \$ => $ in the example such that it reads: sub is_function_in_namespace($$)
Created attachment 203294 [details] Patch
Comment on attachment 203294 [details] Patch Clearing flags on attachment: 203294 Committed r150939: <http://trac.webkit.org/changeset/150939>
All reviewed patches have been landed. Closing bug.
Thanks!