[Webkit-unassigned] [Bug 27354] cpplint should check for one line control statements surrounded by braces

[bugzilla-daemon at webkit.org]

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


David Levin <levin at chromium.org> changed:

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




--- Comment #2 from David Levin <levin at chromium.org>  2009-07-16 15:27:23 PDT ---
(From update of attachment 32895)
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.

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