Bug 30362 – check-webkit-style is wrong about indent checking in namespaces in header files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Carol Szabo]

check-webkit-style has not been updated to reflect the new style-guidelines referring to not indenting in a namespace even in a header file.
Also check-webkit-style was purposefully coded to not warn about missing spaces between numbers and binary operators such as <<, which is not conforming to the webkit style guidelines.

[Carol Szabo]

Created attachment 41179 [details]
Proposed Patch

This patch addresses some of the issues seen with check-webkit-style
It has limited handling of preprocessor directives, thus it may generate a few false errors when a #if directive with #else clause is used at the end of a statement such as here:
namespace test {
void f(
#if HAVE(LONG_LONG_INT)
    long long int);
#else
    long int);
#endif
}

It will complain about the spacing before the 6th line since it will think that the function declaration has finished on the 4th line.

But it addresses many false errors about indentation in header files containing the namespace reserved word. Also it improves handling of spacing around binary operators.

[David Levin]

I should be able to look at this patch in a few hours.

But I'm concerned about supporting this style:

namespace test {
void f(
#if HAVE(LONG_LONG_INT)
    long long int);
#else
    long int);
#endif
}

Code like this breaks a several things 
* indenting in editor (like emacs for example) 
* makes any interline checks in check-webkit-style more fragile as you're noting.

Personally, rather than change check-webkit-style, I'd much prefer discourage this coding pattern in favor of any of the following:

namespace test {
void f(
#if HAVE(LONG_LONG_INT)
    long long int
#else
    long int
#endif
);
}
or

namespace test {
#if HAVE(LONG_LONG_INT)
void f(long long int);
#else
void f(long int);
#endif
}

or

namespace test {
#if HAVE(LONG_LONG_INT)
typedef long long int ReallyLongInt;
#else
typedef long int ReallyLongInt;
#endif
void f(ReallyLongInt);
}

I'd almost recommend putting something in check-webkit-style to avoid the given style but first that would have to be discussed on webkit-dev.

Comments?

[Carol Szabo]

(In reply to comment #2)
> I should be able to look at this patch in a few hours.
> 
> But I'm concerned about supporting this style:
> 
> namespace test {
> void f(
> #if HAVE(LONG_LONG_INT)
>     long long int);
> #else
>     long int);
> #endif
> }
> 
> Code like this breaks a several things 
> * indenting in editor (like emacs for example) 
> * makes any interline checks in check-webkit-style more fragile as you're
> noting.
> 
> Personally, rather than change check-webkit-style, I'd much prefer discourage
> this coding pattern in favor of any of the following:
> 
> namespace test {
> void f(
> #if HAVE(LONG_LONG_INT)
>     long long int
> #else
>     long int
> #endif
> );
> }
> or
> 
> namespace test {
> #if HAVE(LONG_LONG_INT)
> void f(long long int);
> #else
> void f(long int);
> #endif
> }
> 
> or
> 
> namespace test {
> #if HAVE(LONG_LONG_INT)
> typedef long long int ReallyLongInt;
> #else
> typedef long int ReallyLongInt;
> #endif
> void f(ReallyLongInt);
> }
> 
> I'd almost recommend putting something in check-webkit-style to avoid the given
> style but first that would have to be discussed on webkit-dev.
> 
> Comments?

Then its good because my patch flags that code (despite the fact that that was not the intent.

[David Levin]

Comment on attachment 41179 [details]
Proposed Patch

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
> [phone]  Carol Szabo  <[e-mail]>
> +
> +        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.

[David Levin]

(In reply to comment #3)
> Then its good because my patch flags that code (despite the fact that that was
> not the intent.

Cool. I misread your comment #1 when I read it quickly. Now I understand.

[David Levin]

fyi, I just r+ a fix for the same issue (namespace indentation) that was pretty
minimal in scope (https://bugs.webkit.org/show_bug.cgi?id=30426). I still think
there is a lot of value on what you did here with some fixes as mentioned
above. You can still basically replace the code as you are doing.

[Carol Szabo]

Created attachment 41291 [details]
Proposed Patch

This addresses Levin's comments.
I have run the check-webkit-style scripth before and after my changes through the entire WebKit source and I have spot checked issues. At the moment other than the scenario described earlier in the comments, which I understand not to be a favored code style anyway, I saw no false warnings.

[David Levin]

Comment on attachment 41291 [details]
Proposed Patch


> Index: WebKitTools/Scripts/modules/cpp_style.py
> +    if current_indentation_level > 0:
> +        error(filename, line_number, 'whitespace/indent', 4,
> +                          'namespace should never be indented.')

The indentation is off here.

I'll fix this on landing.

[David Levin]

Committed as http://trac.webkit.org/changeset/49690