[PATCH] [clang-tidy] Add check misc-braces-around-statements.

[Alexander Kornienko]

>>! In D5395#36, @curdeius wrote:
> Hi Alex,
> I've done some further modifications to correctly handle statements with a variable declaration in it (like `while (auto x = f()) ;`) and handling macro-expanded statements (ignoring them).
> If you think that it's ok now, can you please land this patch (I don't have write access).

Hi Marek,

The check looks good now! I've made a few minor changes (spelling, comments, braces, etc.) and committing the patch now. I've added a few inline comments to document what I'm changing in the patch and why.

I've also added a FIXME to add some configurability to the check to make it useful for Google and LLVM. Both allow if/while/... bodies without braces with certain conditions, so it would be incorrect to just insert braces everywhere.

Thank you again for the great work!

================
Comment at: clang-tidy/misc/BracesAroundStatementsCheck.cpp:30
@@ +29,3 @@
+
+  if (Invalid) {
+    return tok::NUM_TOKENS;
----------------
Here and elsewhere: we normally don't use braces around single-line if/while/for/... bodies. It's not explicitly written down, but all examples in the Coding Standards document are written this way: http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

================
Comment at: clang-tidy/misc/BracesAroundStatementsCheck.cpp:49
@@ +48,3 @@
+    }
+    // Fast-forward current token
+    Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, Context->getLangOpts());
----------------
Here and elsewhere: the comments should be correct sentences, in particular, start with a capital character and end with a period.

================
Comment at: clang-tidy/misc/BracesAroundStatementsCheck.cpp:54
@@ +53,3 @@
+
+bool isBlockComment(SourceRange R, const SourceManager &SM,
+                    const ASTContext *Context) {
----------------
These two methods do a very similar stuff and are only used once. I've inlined them.

================
Comment at: clang-tidy/misc/BracesAroundStatementsCheck.cpp:62
@@ +61,3 @@
+
+bool hasNewLine(SourceRange R, const SourceManager &SM,
+                const ASTContext *Context) {
----------------
nit: newline is a single word, hence s/hasNewLine/hasNewline/

================
Comment at: clang-tidy/misc/BracesAroundStatementsCheck.cpp:112
@@ +111,3 @@
+
+    assert(TokKind == tok::comment);
+    SourceLocation TokEndLoc =
----------------
It's a bit too defensive. The condition is checked in the previous statement.

================
Comment at: clang-tidy/misc/BracesAroundStatementsCheck.cpp:121
@@ +120,3 @@
+      }
+      // else: One-line block comment, brace after new line
+    }
----------------
nit: s/new line/newline/

================
Comment at: test/clang-tidy/misc-braces-around-statements.cpp:177
@@ +176,3 @@
+
+  // CHECK-MESSAGES: {{clang-tidy applied [0-9]+ of [0-9]+ suggested fixes.}}
+}
----------------
There's no need to check this line.

================
Comment at: unittests/clang-tidy/MiscModuleTest.cpp:78
@@ +77,3 @@
+TEST(BracesAroundStatementsCheck, If) {
+  // if only
+  EXPECT_NO_CHANGES(BracesAroundStatementsCheck, "int main() {\n"
----------------
These comments document facts that are obvious from a single glance at the code. I've removed the comments.

================
Comment at: unittests/clang-tidy/MiscModuleTest.cpp:117
@@ +116,3 @@
+                "}"));
+//EXPECT_EQ("#define EMPTY_MACRO\n"
+//          "int main() {\n"
----------------
We don't commit commented-out code. I've added a FIXME and a test showing current behavior.

http://reviews.llvm.org/D5395