[webkit-reviews] review denied: [Bug 27354] cpplint should check for one line control statements surrounded by braces : [Attachment 32895] Add the cpplint check

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Adam Treat <treat at kde.org>'s
request for review:
Bug 27354: cpplint should check for one line control statements surrounded by
braces
https://bugs.webkit.org/show_bug.cgi?id=27354

Attachment 32895: Add the cpplint check
https://bugs.webkit.org/attachment.cgi?id=32895&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.