Bug 27354 – cpplint should check for one line control statements surrounded by braces

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Adam Treat]

cpplint should check for this:

One-line control clauses should not use braces unless comments are included or a single statement spans multiple lines.
Right:
if (condition)
    doIt();

if (condition) {
    // Some comment
    doIt();
}

if (condition) {
    myFunction(reallyLongParam1, reallyLongParam2, ...
        reallyLongParam5);
}
Wrong:
if (condition) {
    doIt();
}

if (condition)
    // Some comment
    doIt();

if (condition)
    myFunction(reallyLongParam1, reallyLongParam2, ...
        reallyLongParam5);

[Adam Treat]

Created attachment 32895 [details]
Add the cpplint check

[David Levin]

Comment on attachment 32895 [details]
Add the cpplint check

This is awesome!  This is another issue happens fairly frequently and would be nice to tell people about ahead of their review.

I just have a few things to address/respond to, and then we'll be done.


> diff --git a/WebKitTools/Scripts/modules/cpplint.py b/WebKitTools/Scripts/modules/cpplint.py
> +        previous_line = get_previous_non_blank_line(clean_lines, line_number-1)[0]

I don't think using "get_previous_non_blank_line" is quite right because it would flag this case
  if (condition) {
      // Some comment
      doIt();
  }
as needing to get right of the braces and the braces should remain in this case.  I wonder if it should just do this instead:

clean_lines.elided[line_number - 2]


> +        if (previous_line.find('{') > 0
> +            and search (r'\b(if|for|while|switch)\b', previous_line)):

1. Remove space after search.
2. Remove "switch"
3. Add "else"


> +            error(filename, line_number, 'whitespace/braces', 4,

At first this didn't seem like it fit the category of "whitespace" (which in my mind is adding deleting spaces/blank lines).  I was going to suggest "readability/"
However after further consideration, getting rid of braces decreases blank lines so it actually does seem concerned with that after all.


> +                  'This } does not belong as one line control clauses should not use braces')

It seems odd to say that the "}" doesn't belong.  How about just going with something very close to style guide text: "Single-line control clauses should not use braces." ?



> diff --git a/WebKitTools/Scripts/modules/cpplint_unittest.py b/WebKitTools/Scripts/modules/cpplint_unittest.py
> +        self.assert_multi_line_lint(
> +            'if (true) {\n'
> +            '    int foo;\n'
> +            '}\n',
> +            'This } does not belong as one line control clauses should not use braces  [whitespace/braces] [4]')
> +

1. Add a test cases for: for,while,else
2. Add a test cases for:
a.  if (condition) {
        // Some comment
        doIt();
   }

b. if (condition) {
       myFunction(reallyLongParam1, reallyLongParam2,
                  reallyLongParam3);
   }

That verifies that these cases don't get flagged.

[Adam Treat]

Created attachment 32902 [details]
Second version

[David Levin]

Comment on attachment 32902 [details]
Second version


> diff --git a/WebKitTools/Scripts/modules/cpplint.py b/WebKitTools/Scripts/modules/cpplint.py
> +    if (match(r'\s*}\s*$', line)
> +        and line_number > 1):

You might as well put this on one line.  In general, feel free to make the lines as long as you wish.  (Most of the code was developed using 80 characters limits so sometimes it seems to do weird line wrappings to meet this requirement.)


> +        # We check if a closed brace has started a line to see if a
> +        # one line control statement was previous

Nice to add a "." to the end of your sentence.

> +        previous_line = clean_lines.elided[line_number - 2]
> +        if (previous_line.find('{') > 0
> +            and search(r'\b(if|for|while|else)\b', previous_line)):
> +            error(filename, line_number, 'whitespace/braces', 4,
> +                  'One line control clauses should not use braces')

Nice to add a "." to the end of your sentence.

[Adam Treat]

Landed with r46003.