[Webkit-unassigned] [Bug 27461] Add checks for namespace indentation to cpplint

[bugzilla-daemon at webkit.org]

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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33111|review?                     |review-
               Flag|                            |




--- Comment #6 from David Levin <levin at chromium.org>  2009-07-20 17:36:56 PDT ---
(From update of attachment 33111)
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.)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.