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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=113221


Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #203263|review?, commit-queue?      |review+, commit-queue-
               Flag|                            |




--- Comment #26 from Daniel Bates <dbates at webkit.org>  2013-05-29 15:04:17 PST ---
(From update of attachment 203263)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.