[webkit-reviews] review denied: [Bug 27461] Add checks for namespace indentation to cpplint : [Attachment 33111] Add checks for namespace indentation to cpplint (try 3)

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jakob Petsovits <jpetso at gmx.at>'s
request for review:
Bug 27461: Add checks for namespace indentation to cpplint
https://bugs.webkit.org/show_bug.cgi?id=27461

Attachment 33111: Add checks for namespace indentation to cpplint (try 3)
https://bugs.webkit.org/attachment.cgi?id=33111&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.)