[Webkit-unassigned] [Bug 30362] check-webkit-style is wrong about indent checking in namespaces in header files

[bugzilla-daemon at webkit.org]

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


David Levin <levin at chromium.org> changed:

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




--- Comment #4 from David Levin <levin at chromium.org>  2009-10-15 19:53:05 PDT ---
(From update of attachment 41179)
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.

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