REGRESSION(r105797): prepare-ChangeLog for a .cpp file can output an empty method name (i.e. "()")

[Kentaro Hara]

r105797 tried to detect a change outside methods, but it causes a bug that prepare-ChangeLog can output an empty method name, like this:

* foo/bar/baz.cpp:
(method1):
():
(method2):

This is because the cpp parser in prepare-ChangeLog cannot distinguish '{' as the beginning of a method with '{' as the beginning of an array definition at the top level.

int a = { 1, 2, 3 };  // This '{' is the beginning of an array definition.

void func() { // This '{' is the beginning of a method.
    ...;
}

[Kentaro Hara]

Created attachment 124541 [details]
Patch

[Daniel Bates]

Comment on attachment 124541 [details]
Patch

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[]"

[Kentaro Hara]

Created attachment 124634 [details]
Patch

[Kentaro Hara]

Thanks, Dbates!

(In reply to comment #2)
> (From update of attachment 124541 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124541&action=review
> 
> 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";

Added.

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

Oops, fixed.

> I'm r-'ing this patch because ..... the regular expression we are using on line 758 of Tools/Scripts/prepare-ChangeLog isn't the most straightforward to read.

Any good idea to improve the regular expression? This patch just added '=' to the existing regular expression though.

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

I added FIXME and more comment. But I am not sure if this is FIXME. As I commented, the parsers are not intended to implement real parsers but intended to just retrieve function names for most practical syntaxes.

> > 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[]"

Fixed.

[Darin Adler]

Comment on attachment 124634 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124634&action=review

> Tools/ChangeLog:33
> +        (generateFunctionLists): As a hack, modified so that prepare-ChangeLog does not output
> +        an empty method name. Ideally this should not happen but may happen, since the
> +        parsers are not perfect.

I think this is a good idea.

[WebKit Review Bot]

Comment on attachment 124634 [details]
Patch

Clearing flags on attachment: 124634

Committed r106315: <http://trac.webkit.org/changeset/106315>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.