Bug 27461 – Add checks for namespace indentation to cpplint

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jakob Petsovits]

cpplint currently does not check the namespace indentation style rules (#3 and #4 in "Indentation" at http://webkit.org/coding/coding-style.html), and my first patch for WebKit is tackling that. Note that the check for "no indentation in implementation files" only checks the first non-empty line, because otherwise a much more complicated solution would need to be written. The way this patch does it will work in most cases and should not produce false positives.

[Jakob Petsovits]

Created attachment 33106 [details]
Checks for namespace indentation to cpplint

[Jakob Petsovits]

Created attachment 33107 [details]
Add checks for namespace indentation to cpplint (try 2)

Sorry, I forgot an obsolete comment in there (the checks now actually do cover namespaces in implementation files).
Also, adding the copyright for Torch Mobile, as Bugzilla and Adam Treat suggest.

[David Levin]

Did you mean to mark this for review -> r?

[Jakob Petsovits]

Created attachment 33111 [details]
Add checks for namespace indentation to cpplint (try 3)

One more try, this time updating the method apidox (had a copy-n-paste error in it) and being nice to preprocessor instructions.
Other parts like "using namespace Foo;" might also get exceptions, but then those are standard C++ code and should probably be treated like all other C++ code as well. So not building any special cases for those into the code.

[Jakob Petsovits]

@David: Didn't I mark it as r? (Flagging just the attachment is enough, right? Unfortunately with the updated patch I don't see whether the previous one had been marked or not, maybe I forgot to mark that one.)

[David Levin]

Comment on attachment 33111 [details]
Add checks for namespace indentation to cpplint (try 3)

Cool.  Thanks for adding this.  There are a few things to address below at this point.


> diff --git a/WebKitTools/Scripts/modules/cpplint.py b/WebKitTools/Scripts/modules/cpplint.py
>  # Copyright (c) 2009 Google Inc. All rights reserved.
> +# Copyright (c) 2009 Torch Mobile Inc.

Not necessary but you would make me happy if you changed the (c) to (C) in both of these (and in the other file).

> +def check_namespace_indentation(filename, clean_lines, line_number, file_extension, error):
...
> +    line = clean_lines.elided[line_number]        # get rid of comments and strings

Just one space before the #.


> +    if not match(r'\s*namespace\s+\S+\s+{\s*$', line):
I would change this to 
   namespace_match = match(r'(?P<namespace_indentation>\s*)namespace\s+\S+\s*{\s*$', line):
   if not namespace_match:

I did two changes:
1. I made "namespace WebKit{" match so that this code can catch things even if there is a bad style.
2. I added a named group which allows the next change:

> +    namespace_indentation = line[0 : line.find('namespace')]

namespace_indentation = namespace_match.group('namespace_indentation')

> +    if file_extension == 'h':
> +        is_implementation_file = False
> +    elif file_extension == 'cpp' or file_extension == 'c' or file_extension == 'mm':
> +        is_implementation_file = True
> +    else:
> +        return
> +
> +    is_header_file = not is_implementation_file

Note that cpplint only processes files with certain extensions already, so there is no need to do the filtering again here.

With that in mind, consider this
    is_header_file = file_extension == 'h'
    is_implementation_file = not is_header_file

> +    line_offset = 0
> +
> +    if is_header_file:
> +        inner_indentation = namespace_indentation + '    '

  inner_indentation = namespace_indentation + ' ' * 4
Makes it clearer that it is 4 spaces.

> +        for current_line in clean_lines.raw_lines[line_number + 1 : clean_lines.num_lines()]:

No spaces around ":"
Also, for indexes at the edges, you don't need to list them, so you should do this

for current_line in clean_lines.raw_lines[line_number + 1:]:

> +            # Skip not only empty lines but also those with preprocessor directives.

What about labels? (Not case labels but other labels.)

> +            if current_line.strip() == '' or current_line.startswith('#'):
> +                continue
> +
> +            if current_line.find(inner_indentation) != 0:

if not current_line.startswith(inner_indentation):

> +                # If something unindented was discovered, make sure it's a closing brace.
> +                if current_line.find(namespace_indentation + '}') != 0:

if not current_line.startswith(namespace_indentation + '}'):

> +                    error(filename, line_number + line_offset, 'whitespace/indent', 4,
> +                          'In a header, code inside a namespace should be indented.')
> +                break
> +
> +    if is_implementation_file:
> +        for current_line in clean_lines.raw_lines[line_number + 1 : clean_lines.num_lines()]:

Use
        for current_line in clean_lines.raw_lines[line_number + 1:]:

> +            line_offset += 1
> +
> +            if current_line.strip() == '':
> +                continue
> +
> +            remaining_line = current_line[len(namespace_indentation) : len(current_line)]

Use
            remaining_line = current_line[len(namespace_indentation):]

> +            if not match(r'\S+', remaining_line):

Why bother with the +?
 if not match(r'\S', remaining_line):


> diff --git a/WebKitTools/Scripts/modules/cpplint_unittest.py b/WebKitTools/Scripts/modules/cpplint_unittest.py
> +    def assert_multi_line_lint(self, code, expected_message, file_name = 'foo.h'):

No spaces around = for arguments (weird python style rule).

> +    def assert_multi_line_lint_re(self, code, expected_message_re, file_name = 'foo.h'):

No spaces around = for arguments (weird python style rule).


> @@ -275,6 +277,64 @@ class CpplintTest(CpplintTestBase):
>              'Line ends in whitespace.  Consider deleting these extra spaces.'
>              '  [whitespace/end_of_line] [4]')
>  
> +    def test_namespace_indentation(self):

These tests should go in class WebKitStyleTests:, def_test_indentation: Look for "# 3." and "# 4." to see where to add them.

> +        # Test indentation in header files, which is required.
> +        self.assert_multi_line_lint(
> +            'namespace WebCore {\n'

Add an empty line here to test that code as well.

> +            '    class Document {\n'

Would be nice to add a line there as well to verify that lines indented further in the namespace are not flagged.

> +            '    };\n'
> +            '}',
> +            '',
> +            'foo.h')


> +
> +        # Test indentation in implementation files, which is not allowed.
> +        self.assert_multi_line_lint(
> +            'namespace WebCore {\n'

Add an empty line here to test that code as well.

> +            'Document::Foo() {}\n'

Please make this a function with contents to verify that it doesn't flag the lines indented inside of the function.


> +            '}',
> +            '',
> +            'foo.cpp')
> +        self.assert_multi_line_lint(
> +            'namespace OuterNamespace {\n'
> +            'namespace InnerNamespace {\n'
> +            'Document::Foo() {}\n'

Please add a space in side of { } (because I think this is the preferred WebKit style and the tests might as well reflect that).
(The same thing in other places where {} appears.)

[David Levin]

btw, I wonder if this functionality would be better as something which processed the whole file at once (like the functions called from process_file_data()).

If written that way, I could see this function more easily morphing (in the future) into something which verified the indent of every line.  (Also it just feels more natural given that it loops over all of the lines after it hits a namespace.)

What do you think about this?

[Jakob Petsovits]

Thanks for the detailed review! Here's a new patch addressing the points from comments #6.

> ...
> > +    line = clean_lines.elided[line_number]        # get rid of comments and strings
>
> Just one space before the #.

Actually that line was copied from the function below, so the updated patch fixes both my line and the original one.

> > +            # Skip not only empty lines but also those with preprocessor directives.
>
> What about labels? (Not case labels but other labels.)

Good point, I added a check for those too - if they're at the start of a line, otherwise that's just weird, or a switch label. As the loop operates on raw lines (in order to catch indentation of comments as well), I didn't match until the end of the string "$" but only until the colon, hopefully it'll be ok this way. If not, I will need to loop over both raw and elided lines in parallel.

> btw, I wonder if this functionality would be better as something which
> processed the whole file at once (like the functions called from
> process_file_data()).
>
> If written that way, I could see this function more easily morphing (in the
> future) into something which verified the indent of every line.  (Also it just
> feels more natural given that it loops over all of the lines after it hits a
> namespace.)
>
> What do you think about this?

It might make sense in the long term if we want to build in further, more detailed checks. But checking the indent of every line also has a big downside, as such a "continuous" check needs to keep track of indentation state throughout all the lines that it checks. For example, my current patch avoids this situation for indentation files by only checking the first line after the namespace opens, because detecting the actual ending brace for the namespace is way over my head without some kind of at least a half-decent C++ parser. With an all-at-once approach, this kind of "hack" is not an option.

We'd also need to build in manual nesting support (e.g. for nested namespaces, or switching from a namespace context to a class context and back) which the current check can avoid because it checks each context independently from each other.

So while an all-at-once approach has the potential to improve the quality of indentation checks in general, I think it's a larger undertaking and might necessitate more complexity than what's in scope of this patch. imho.

[Jakob Petsovits]

Created attachment 33178 [details]
Add checks for namespace indentation to cpplint (try 4)

See comment above, addresses various issues that were highlighted by David Levin.

[Jakob Petsovits]

Created attachment 33181 [details]
Add checks for namespace indentation to cpplint (try 5)

Adapt checks for goto labels as discussed with Dave on IRC: empirical research shows that such labels are not used in header files, and therefore don't need to be checked. Also, add a test case for goto labels in implementation files, just to make sure the check actually works (although it probably won't be triggered anytime soon, because indented namespaced hardly exist anywhere in implementation files).

[David Levin]

Comment on attachment 33181 [details]
Add checks for namespace indentation to cpplint (try 5)

It could be landed as is.  If the lander would fix these issues, it would be even better.


> diff --git a/WebKitTools/Scripts/modules/cpplint.py b/WebKitTools/Scripts/modules/cpplint.py
> +def check_namespace_indentation(filename, clean_lines, line_number, file_extension, error):
> +    """Looks for indentation errors inside of namespaces.
> +
> +    Args:
> +      filename: The name of the current file.
> +      clean_lines: A CleansedLines instance containing the file.
> +      line_number: The number of the line to check.
> +      file_extension: The extension (dot not included) of the file.
> +      error: The function to call with any errors found.
> +    """
> +
> +    line = clean_lines.elided[line_number] # get rid of comments and strings

It is nice to make comment look like sentences.  Start with a capital. End with a period.  (I know this was copied from elsewhere in the file.)


> +    if is_implementation_file:
> +        for current_line in clean_lines.raw_lines[line_number + 1:]:
> +            line_offset += 1
> +
> +            # Skip not only empty lines but also those with (goto) labels.

You should be able to hit a goto label before you hit other code and break out of this, but no big deal.  Maybe it is useful to leave this in case we decide to extend this function to not have the break.


> diff --git a/WebKitTools/Scripts/modules/cpplint_unittest.py b/WebKitTools/Scripts/modules/cpplint_unittest.py

> +        self.assert_multi_line_lint(
> +            'namespace WebCore {\n\n'
> +            'bool Document::Foo() {}\n'
> +            '    return true;\n'

It would be nice to have valid code.  Something like this:

            'bool Document::Foo()\n'
            '{\n'
            '    return true;\n'
            '}',

> +            '}',
> +            '',
> +            'foo.cpp')
> +        self.assert_multi_line_lint(
> +            'namespace OuterNamespace {\n'
> +            'namespace InnerNamespace {\n'
> +            'Document::Foo() { }\n'
> +            '}',
> +            '',
> +            'foo.cpp')
> +        self.assert_multi_line_lint(
> +            '    namespace WebCore {\n\n'
> +            '    void Document::Foo() {}\n'
> +            'start:  // infinite loops are fun!\n'
> +            '        goto start;\n'
Invalid code.

> +            '    }',
> +            '',
> +            'foo.cpp')

[Jakob Petsovits]

Created attachment 33185 [details]
Add checks for namespace indentation to cpplint (try 6)

Above issues fixed, apart from the one more goto label stuff. Newbie question: is it ok to repost a new patch as r+ if it was already reviewed with r+ before (and I only made slight changes like these)?

[David Levin]

Comment on attachment 33185 [details]
Add checks for namespace indentation to cpplint (try 6)

Sure posting a new patch like this is fine.  (It would be easier if there was diff between patches :) but that may come one day in the not too distant future).

[Adam Treat]

Landed with r46182.