Bug 113221 – prepare-Changelog should not be generating namespace-only or class-name-only lines like "(WebCore):"

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Philip Rogers]

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.

[Alexey Proskuryakov]

*** Bug 116439 has been marked as a duplicate of this bug. ***

[Darin Adler]

prepare-ChangeLog is supposed to list function names. Not namespace and class names.

[Ruth Fong]

Created attachment 202632 [details]
Patch

[Tim Horton]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Ruth Fong]

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).

[Ruth Fong]

Created attachment 203111 [details]
Patch

[Ryosuke Niwa]

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.".

[Ryosuke Niwa]

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.

[Ruth Fong]

Created attachment 203212 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/14013385>

[Ryosuke Niwa]

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".

[Daniel Bates]

Is it not possible to omit these these namespace/struct/class entries from the @ranges array when we build it?

[Ruth Fong]

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.

[Build Bot]

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

[Build Bot]

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

[Tim Horton]

(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.

[Ruth Fong]

Created attachment 203263 [details]
Patch

[Daniel Bates]

(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?

[Build Bot]

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

[Build Bot]

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

[Ruth Fong]

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.

[Daniel Bates]

(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).

[Daniel Bates]

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.

[Daniel Bates]

(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($$)

[Ruth Fong]

Created attachment 203294 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 203294 [details]
Patch

Clearing flags on attachment: 203294

Committed r150939: <http://trac.webkit.org/changeset/150939>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Thanks!