[webkit-reviews] review denied: [Bug 30362] check-webkit-style is wrong about indent checking in namespaces in header files : [Attachment 41179] Proposed Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Carol Szabo
<carol.szabo at nokia.com>'s request for review:
Bug 30362: check-webkit-style is wrong about indent checking in namespaces in
header files
https://bugs.webkit.org/show_bug.cgi?id=30362

Attachment 41179: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=41179&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Thanks for taking care of this! Just a few things to address and this will be
in.

Something that I think is pretty important when changing this tool: Did you try
running this over the current WebKit code base to ensure that it doesn't
introduce (lots of) false positives?


> Index: WebKitTools/ChangeLog
> +2009-10-14  Carol Szabo  <carol.szabo at nokia.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +

Ideally this would have:
   bug title
   link to bug

   description

You have these elements in here but not quite in this format.

> +	   check-webkit style is wrong about indent checking in namespaces in
> +	   header files also it does not allow spaces around the equal sign
> +	   inside 'if' statements and around binary operators that take 
> +	   numeric literals and it reports false errors for the / operator
> +	   when part of a filename in the #include directive.

It would be nice to add some punctuation in here. Also, there are a lot of
"and"s joining sentences which would be nice to break up.


> +	   https://bugs.webkit.org/show_bug.cgi?id=30362
> +
> +	   * Scripts/modules/cpp_style.py:
> +	   Improved indentation checking and space checking around
> +	   binary operators. While the checks are still not perfect,
> +	   they are clearly better than before.
> +	   * Scripts/modules/cpp_style_unittest.py:
> +	   Added test cases for the newly supported checks and modified old
> +	   test cases to match the new guidelines

Nice explanations.

> Index: WebKitTools/Scripts/modules/cpp_style.py
> @@ -1533,16 +1533,15 @@ def check_spacing(filename, clean_lines,


> +    if search(r'[\w.]=[\w.]', line):
>	   error(filename, line_number, 'whitespace/operators', 4,
> -		 'Missing spaces around =')
> +	       'Missing spaces around =')

This change in spacing is incorrect.

> @@ -1699,50 +1696,34 @@ def check_namespace_indentation(filename

> +    looking_for_semicolon = False;
>      line_offset = 0
> +    in_preprocessor_directive = False;
> +    for current_line in clean_lines.elided[line_number + 1:]:
> +	   line_offset += 1

> +	   if current_line.strip() == '':
Use
	if not current_line.strip():

> +	       continue
> +	   if not current_indentation_level:
> +	       if not (in_preprocessor_directive or looking_for_semicolon):
> +		   if not match(r'\S', current_line):
>		       error(filename, line_number + line_offset,
'whitespace/indent', 4,
> +			   'Code inside a namespace should not be indented.')

This isn't indented correctly. (It should use the open paren on the previous
line.)

> +	       if in_preprocessor_directive or (current_line.strip()[0] ==
'#'):  # This takes care of preprocessor directive syntax

Please put one space before end of line comments and end them with punctuation.


> +		   in_preprocessor_directive = current_line[-1] == '\\'
> +	       else:
> +		   looking_for_semicolon = ((current_line.find(';') == -1) and
(current_line.strip()[-1] != '}')) or (current_line[-1] == '\\')
> +	   else:
> +	       looking_for_semicolon = False; # if we have a brace we may not
need a semicolon

s/if we have a brace we may not need a semicolon/If we have a brace, we may not
need a semicolon./


> Index: WebKitTools/Scripts/modules/cpp_style_unittest.py

Where are the tests for these items?

Also, it does not allow spaces 
   * around the equal sign inside 'if' statements and 
   * around binary operators that take numeric literals.

It reports false errors for the / operator when part of a filename in the
#include directive.