Bug 32597 – Check one space before end of line comments

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

check-webkit-style should check one space before end of line comments.

[Fumitoshi Ukai]

Created attachment 44946 [details]
Check one space before end of line comments.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44946 [details] without any errors.

[Adam Barth]

Why are these warning off by default?  They seem like awesomesauce.

[Fumitoshi Ukai]

(In reply to comment #3)
> Why are these warning off by default?  They seem like awesomesauce.

These warning is off by default in cpp_style.py, but enabled by default in check-webkit-style (by cpp_style.use_webkit_styles() overriding _DEFAULT_FILTERS)

[Eric Seidel (no email)]

I don't see what the distinction is.  cpp_style.py is fully forked from its original chromium/google origins, so I'm not sure why running cpp_style.py should be different from running run-webkit-tests.

[Fumitoshi Ukai]

(In reply to comment #5)
> I don't see what the distinction is.  cpp_style.py is fully forked from its
> original chromium/google origins, so I'm not sure why running cpp_style.py
> should be different from running run-webkit-tests.

Hmm, so we don't need use_webkit_styles() and should have webkit style defaults in _DEFAULT_FILTERS?
Should we fix the code instead of adding new error categories?(e.g. fix whitespace/comments rather than adding whitespace/webkit_comments)

[David Levin]

(In reply to comment #6)
> (In reply to comment #5)
> > I don't see what the distinction is.  cpp_style.py is fully forked from its
> > original chromium/google origins, so I'm not sure why running cpp_style.py
> > should be different from running run-webkit-tests.
> 
> Hmm, so we don't need use_webkit_styles() and should have webkit style defaults
> in _DEFAULT_FILTERS?

Maybe so.

> Should we fix the code instead of adding new error categories?(e.g. fix
> whitespace/comments rather than adding whitespace/webkit_comments)

Yes. (That fix possibly could be just replacing it with what you did -- I haven't had a chance to review but was planning to look later tonight -- along with the worker/websocket patch).

[Chris Jerdonek]

(In reply to comment #6)

> Hmm, so we don't need use_webkit_styles() and should have webkit style defaults
> in _DEFAULT_FILTERS?

You should update since check-webkit-style changed since your patch.

My thinking is that the webkit default filter rules formerly in use_webkit_styles() should move from cpp_style.py to the check-webkit-style executable, and they should be passed in as a parameter to a function in cpp_style.py.  That's the direction I'm moving in:

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

I also think we should phase out the _DEFAULT_FILTERS global variable (now called _DEFAULT_FILTER_RULES), but we can't do that quite yet since the unit tests use it if I remember right.

[Fumitoshi Ukai]

(In reply to comment #8)
> (In reply to comment #6)
> 
> > Hmm, so we don't need use_webkit_styles() and should have webkit style defaults
> > in _DEFAULT_FILTERS?
> 
> You should update since check-webkit-style changed since your patch.
> 
> My thinking is that the webkit default filter rules formerly in
> use_webkit_styles() should move from cpp_style.py to the check-webkit-style
> executable, and they should be passed in as a parameter to a function in
> cpp_style.py.  That's the direction I'm moving in:

It sounds different direction as Eric and David said.
I think we'll remove use_webkit_styles() and make check-webkit-style act as the same way as cpp_style.py by default.
If we pass filters in use_webkit_styles() from check-webkit-style to cpp_style.py, then it means cpp_style.py works differently from check-webkit-style unless we pass the same filters, doesn't it?

> 
> https://bugs.webkit.org/show_bug.cgi?id=32592
> 
> I also think we should phase out the _DEFAULT_FILTERS global variable (now
> called _DEFAULT_FILTER_RULES), but we can't do that quite yet since the unit
> tests use it if I remember right.

We use change semantics of some error categories and/or _DEFAULT_FILTERS, we need to update unittest as well.
(That's why I preserved default error categories in cpp_style.py in the first patch.)

[Chris Jerdonek]

(In reply to comment #9)

> If we pass filters in use_webkit_styles() from check-webkit-style to
> cpp_style.py, then it means cpp_style.py works differently from
> check-webkit-style unless we pass the same filters, doesn't it?

No, because cpp_style.py will never be running as an executable.  If you look at the recent changes, we also removed the main() function from cpp_style.py.  The file cpp_style.py will just be providing functionality for the command-line script check-webkit-style to call.

You can read the discussion here for more background:

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

[Fumitoshi Ukai]

Created attachment 45043 [details]
Check one space before end of line comments.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45043 [details] without any errors.

[Shinichiro Hamaji]

Comment on attachment 45043 [details]
Check one space before end of line comments.

Some comments... First of all, indent level should be 4, not 2.

> @@ -874,8 +872,7 @@ def get_header_guard_cpp_variable(filename):
>  
>      """
>  
> -    fileinfo = FileInfo(filename)
> -    return sub(r'[-./\s]', '_', fileinfo.repository_name()).upper() + '_'
> +    return sub(r'[-./\s]', '_', os.path.basename(filename))

We don't need to replace '/' anymore?

> @@ -1362,31 +1363,6 @@ class CppStyleTest(CppStyleTestBase):
>          self.assert_multi_line_lint('#endif\n    );',
>                                      '')
>  
> -    def test_two_spaces_between_code_and_comments(self):
> -        self.assert_lint('} // namespace foo',
> -                         'At least two spaces is best between code and comments'
> -                         '  [whitespace/comments] [2]')
> -        self.assert_lint('}// namespace foo',
> -                         'At least two spaces is best between code and comments'
> -                         '  [whitespace/comments] [2]')
> -        self.assert_lint('printf("foo"); // Outside quotes.',
> -                         'At least two spaces is best between code and comments'
> -                         '  [whitespace/comments] [2]')
> -        self.assert_lint('int i = 0;  // Having two spaces is fine.', '')
> -        self.assert_lint('int i = 0;   // Having three spaces is OK.', '')
> -        self.assert_lint('// Top level comment', '')
> -        self.assert_lint('    // Line starts with four spaces.', '')
> -        self.assert_lint('foo();\n'
> -                         '{ // A scope is opening.', '')
> -        self.assert_lint('    foo();\n'
> -                         '    { // An indented scope is opening.', '')
> -        self.assert_lint('if (foo) { // not a pure scope; comment is too close!',
> -                         'At least two spaces is best between code and comments'
> -                         '  [whitespace/comments] [2]')
> -        self.assert_lint('printf("// In quotes.")', '')
> -        self.assert_lint('printf("\\"%s // In quotes.")', '')
> -        self.assert_lint('printf("%s", "// In quotes.")', '')
> -

I don't want to remove this test. They look good. Especially, I think we want to have a testcase for a comment in a string literal.

[David Levin]

Comment on attachment 45043 [details]
Check one space before end of line comments.

Shinichiro did a fine job of reviewing this patch.

r- for the removed test cases that Shinichiro noted.

One small additional comment below.

> diff --git a/WebKitTools/Scripts/modules/cpp_style.py b/WebKitTools/Scripts/modules/cpp_style.py
> +                and ((line[comment_position-1] not in string.whitespace)
> +                     or (line[comment_position-2] in string.whitespace)))):

I know this was a problem before but since you are changing these lines please add spaces around the "-".

[Fumitoshi Ukai]

Created attachment 45123 [details]
Check one space before end of line comments.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45123 [details] without any errors.

[Shinichiro Hamaji]

Thanks for the fixes. Looks good. It seems there are actually two changes in this patch (one space before comment and header guard). It would be better to update the ChangeLog to mention both?

[Fumitoshi Ukai]

Created attachment 45127 [details]
Check one space before end of line comments.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45127 [details] without any errors.

[David Levin]

Comment on attachment 45127 [details]
Check one space before end of line comments.

Thanks!

Minor nit to consider fixing when landing this below.

> diff --git a/WebKitTools/Scripts/modules/cpp_style_unittest.py b/WebKitTools/Scripts/modules/cpp_style_unittest.py
> +    def test_comments(self):
> +        # comment at the begining line is ok.

typo: begining
s/comment/A comment/
s/line/of a line/

[Fumitoshi Ukai]

Committed r52318: <http://trac.webkit.org/changeset/52318>