[webkit-reviews] review granted: [Bug 113221] prepare-Changelog should not be generating namespace-only or class-name-only lines like "(WebCore):" : [Attachment 203263] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Ruth Fong <ruthiecftg at gmail.com>'s
request for review:
Bug 113221: prepare-Changelog should not be generating namespace-only or
class-name-only lines like "(WebCore):"
https://bugs.webkit.org/show_bug.cgi?id=113221

Attachment 203263: Patch
https://bugs.webkit.org/attachment.cgi?id=203263&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
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.