The cpp parser of prepare-ChangeLog cannot detect a change in classes and namespaces

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kentaro Hara]

The cpp parser of prepare-ChangeLog cannot detect a change outside methods, like this:

namespace N {
int a;     // this change does not appear on ChangeLog.
class C {
    int b;     // this change does not appear on ChangeLog.
    void f()
    {
        int c;     // this change appears on ChangeLog.
    }
    int d;     // this change does not appear on ChangeLog.
};
int e;     // this change does not appear on ChangeLog.
};

Current ChangeLog:
    (N::C::f):

Expected ChangeLog:
    (N):
    (N::C):
    (N::C::f):

[Kentaro Hara]

Created attachment 121087 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 121087 [details]
Patch

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

> Tools/ChangeLog:29
> +            (N):

Do we really want that? This will increase the number of lines in each change log ebtry considerably.

[Tony Chang]

(In reply to comment #2)
> (From update of attachment 121087 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121087&action=review
> 
> > Tools/ChangeLog:29
> > +            (N):
> 
> Do we really want that? This will increase the number of lines in each change log entry considerably.

My understanding from reading the bug description was that (N): would only be added if you added something to the namespace itself (in the bug description, this would be 'int a;').

[Kentaro Hara]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 121087 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=121087&action=review
> > 
> > > Tools/ChangeLog:29
> > > +            (N):
> > 
> > Do we really want that? This will increase the number of lines in each change log entry considerably.
> 
> My understanding from reading the bug description was that (N): would only be added if you added something to the namespace itself (in the bug description, this would be 'int a;').

Right. The line appears only when you add something to the class or namespace.

- Python and Java have been also outputing "(N):".
- I've sometimes felt inconvenient because prepare-ChangeLog outputs nothing when I added variables to classes.

[David Kilzer (:ddkilzer)]

Comment on attachment 121087 [details]
Patch

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

r=me, but please look into the duplicate namespace entry issue.

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests-expected.txt:311
> +  [
> +    '299',
> +    '300',
> +    'NameSpace6'
> +  ],
> +  [
> +    '302',
> +    '303',
> +    'NameSpace5'
>    ]

These NameSpace* entries seem to be duplicate/bogus (as are some other new NameSpace* entries.  This doesn't seem to affect the output in the ChangeLog entry (since the duplicate entries are apparently removed), but it seems like something that should be cleaned up if it's easy to do.

[Kentaro Hara]

Comment on attachment 121087 [details]
Patch

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

>> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests-expected.txt:311
>>    ]
> 
> These NameSpace* entries seem to be duplicate/bogus (as are some other new NameSpace* entries.  This doesn't seem to affect the output in the ChangeLog entry (since the duplicate entries are apparently removed), but it seems like something that should be cleaned up if it's easy to do.

ddkilzer: Duplicated ChangeLog entries are removed from the output of prepare-ChangeLog, but the duplication in the output of get_function_line_ranges_for_XXXX() is needed. In case of NameSpace5, we need to somehow tell prepare-ChangeLog that the ranges of NameSpace5 are [283, 284] _and_ [302, 303]. (If a modification happens at line 203 but we just tell prepare-ChangeLog that the range of NameSpace5 is [183, 184], then prepare-ChangeLog would output nothing for NameSpace5.)

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> (From update of attachment 121087 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121087&action=review
> 
> >> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/cpp_unittests-expected.txt:311
> >>    ]
> > 
> > These NameSpace* entries seem to be duplicate/bogus (as are some other new NameSpace* entries.  This doesn't seem to affect the output in the ChangeLog entry (since the duplicate entries are apparently removed), but it seems like something that should be cleaned up if it's easy to do.
> 
> ddkilzer: Duplicated ChangeLog entries are removed from the output of prepare-ChangeLog, but the duplication in the output of get_function_line_ranges_for_XXXX() is needed. In case of NameSpace5, we need to somehow tell prepare-ChangeLog that the ranges of NameSpace5 are [283, 284] _and_ [302, 303]. (If a modification happens at line 203 but we just tell prepare-ChangeLog that the range of NameSpace5 is [183, 184], then prepare-ChangeLog would output nothing for NameSpace5.)

Okay, thanks for the explanation!

[WebKit Review Bot]

Comment on attachment 121087 [details]
Patch

Clearing flags on attachment: 121087

Committed r105797: <http://trac.webkit.org/changeset/105797>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.