Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove curly brace CPP Lint checks #2109

Merged
merged 4 commits into from
Apr 25, 2018
Merged

Remove curly brace CPP Lint checks #2109

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d344dd9
Update coding standard
Apr 24, 2018
692c4f3
Remove brace checking from cpplint
Apr 24, 2018
78191ee
Remove NOLINTs for lambdas.
Apr 24, 2018
18cab61
Rephrase and justify curly brace alignment exceptions
Apr 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CODING_STANDARD.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ Formatting is enforced using clang-format. For more information about this, see
- Nested function calls do not need to be broken up into separate lines
even if the outer function call does.
- If a method is bigger than 50 lines, break it into parts.
- Put matching `{ }` into the same column.
- Put matching `{ }` into the same column, except for lambdas, where you should
place `{` directly after the closing `)`. This rule also doesn't apply to
initializer lists.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment on the rationale of this (convenience rule due to deficiencies of clang-format)? We should revert the rule once the clang-format bug is fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule also doesn't apply to initializer lists. sounds a bit strange to me. I'd remove the also - is it the rule (Put matching...) or the exception (except...) that doesn't apply... a bit confusing. Please reword.

- Spaces around binary operators (`=`, `+`, `==` ...)
- Space after comma (parameter lists, argument lists, ...)
- Space after colon inside `for`
Expand Down
8 changes: 0 additions & 8 deletions scripts/cpplint.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3972,14 +3972,6 @@ def CheckBracesSpacing(filename, clean_lines, linenum, nesting_state, error):
error(filename, linenum, 'whitespace/braces', 5,
'Missing space before {')

# Make sure '} else {' has spaces.
# if Search(r'}else', line):
# error(filename, linenum, 'whitespace/braces', 5,
# 'Missing space before else')
if (Search(r'^.*[^\s].*}$', line) or Search(r'^.*[^\s].*{$', line)) and not(Search(r'{[^}]*}', line)):
error(filename, linenum, 'whitespace/braces', 5,
'Put braces on a separate next line')

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this also remove the check for all compound statements such as loops, non-lambda function bodies, if/else? How about, instead of completely removing it, adding a not(Search(r'\]\s*\(.*\)\s*{', line))?

Copy link
Contributor Author

@LAJW LAJW Apr 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't work in the case of lambda with a return type:

[] (T& t) -> optionalt<std::string> { ...

Or with a lambda with omitted arguments (used in named scopes):

auto value = [&] {
  if (this)
    return 1;
  if (that)
    return 2;
  return 3;
}();

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't be very hard to add expressions for those, but I do worry about the various line breaks that could be used.

# You shouldn't have a space before a semicolon at the end of the line.
# There's a special case for "for" since the style guide allows space before
# the semicolon there.
Expand Down