[webkit-reviews] review denied: [Bug 77336] REGRESSION(r105797): prepare-ChangeLog for a .cpp file can output an empty method name (i.e. "()") : [Attachment 124541] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Kentaro Hara
<haraken at chromium.org>'s request for review:
Bug 77336: REGRESSION(r105797): prepare-ChangeLog for a .cpp file can output an
empty method name (i.e. "()")
https://bugs.webkit.org/show_bug.cgi?id=77336

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=124541&action=review


I'm r-'ing this patch because all the test cases use an invalid syntax for
defining a C array and the regular expression we are using on line 758 of
Tools/Scripts/prepare-ChangeLog isn't the most straightforward to read.

You may want to consider adding test cases for:

1. int a[] = { 1, 2, 3 };
2. int a[3] = { 1, 2, 3 };
3. int a[][3] = { {1, 2, 3}, {4, 5, 6} };
4. int a[2][3] = { {1, 2, 3}, {4, 5, 6} };
5. extern int a[];
6. char a[4] = "test";

> Tools/ChangeLog:21
> +	       int a = { 1, 2, 3 };  // This '{' is the beginning of an array
definition.

Nit: I think you meant to write: int a[] = { 1, 2, 3 }.

(Notice the square brackets after the variable name a).

> Tools/Scripts/prepare-ChangeLog:274
> +		   # This is a hack. The parsers are not perfect and sometimes
function names
> +		   # cannot be retrieved correctly. If the function name is
empty, skip it.

Can you elaborate on why function names may be retrieved incorrectly? At the
very least I suggest that this comment be made into a FIXME comment. You may
also want to consider filing a bug so as to make this FIXME actionable.

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:324
> +static int array1 = { };

"int array1" => "int array1[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:326
> +static int array2 = {

"int array2" => "int array2[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:329
> +static int array3 = { 1, 2, 3 };

"int array3" => "int array3[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:331
> +static int array4 = {

"int array4" => "int array4[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:339
> +static int array5 = { };

"int array5" => int array5[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:341
> +static int array6 = {

"int array6" => "int array6[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:344
> +static int array7 = { 1, 2, 3 };

"int array7" => "int array7[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:346
> +static int array8 = {

"int array8" => "int array8[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:353
> +static int array9 = { };

"int array9" => "int array9[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:355
> +static int array10 = {

"int array10" => "int array10[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:358
> +static int array11 = { 1, 2, 3 };

"int array11" => "int array11[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:360
> +static int array12 = {

"int array12" => "int array12[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:368
> +    static int array13 = { };

"int array13" => "int array13[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:370
> +    static int array14 = {

"int array14" => "int array14[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:373
> +    static int array15 = { 1, 2, 3 };

"int array15" => "int array15[]"

>
Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests.cpp
:375
> +    static int array16 = {

"int array16" => "int array16[]"