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);
Created attachment 32895 [details] Add the cpplint check
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.
Created attachment 32902 [details] Second version
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.
Landed with r46003.