[clang-tidy] misc-braces-around-statements: add option ShortStatementLines to trigger this check only if the statement spans over at least a given number of lines.

[alexfh]

The code is fine. I'm slightly concerned about the code duplication in the tests, but I see no better way of doing this. If we come up with something more elegant, we can always change this. Please address the comments and I'll commit the patch for you.

clang-tidy/misc/BracesAroundStatementsCheck.cpp

117

I'd change the default to 0U in order to use get<unsigned>.

test/clang-tidy/misc-braces-around-statements-always.cpp

6

You should be able to use the "-config" command line option added in r218511 to avoid creating a separate .clang-tidy file.

15

You can omit this, it's added implicitly by the script.

[curdeius]

Use -config to pass CheckOptions.

• [clang-tidy] [misc-braces-around-statements] Using -config for tests instead of a new .clang-tidy file. Default value set to: 0U.
• Merge branch 'master' into feature/misc-braces-around-statements-option

[alexfh]

Committed as r219611 with minor modifications:

merged test/clang-tidy/misc-braces-around-statements-always.cpp into
test/clang-tidy/misc-braces-around-statements.cpp and removed unnecessary
CHECK-NOTs from the tests.

Thanks for the patch!

clang-tidy/misc/BracesAroundStatementsCheck.h

 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_BRACES_AROUND_STATEMENTS_CHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_BRACES_AROUND_STATEMENTS_CHECK_H
 
 #include "../ClangTidy.h"
 
 namespace clang {
 namespace tidy {
 
+/// \brief Checks that bodies of 'if' statements and loops ('for', 'range-for',
+/// 'do-while', and 'while') are inside braces
+///
+/// Before:
+/// if (condition)
+///   statement;
+///
+/// After:
+/// if (condition) {
+///   statement;
+/// }
+///
+/// Additionally, one can define an option `ShortStatementLines` defining the
+/// minimal number of lines that the statement should have in order to trigger
+/// this check.
+/// The number of lines is counted from the end of condition or initial keyword
+/// (do/else) until the last line of the inner statement.
+/// Default value 0 means that braces will be added to all statements (not
+/// having them already).
 class BracesAroundStatementsCheck : public ClangTidyCheck {
 public:
-  BracesAroundStatementsCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  BracesAroundStatementsCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   void checkStmt(const ast_matchers::MatchFinder::MatchResult &Result,
                  const Stmt *S, SourceLocation StartLoc,
                  SourceLocation EndLocHint = SourceLocation());
   template <typename IfOrWhileStmt>
   SourceLocation findRParenLoc(const IfOrWhileStmt *S, const SourceManager &SM,
                                const ASTContext *Context);
+
+private:
+  const unsigned ShortStatementLines;
 };
 
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_BRACES_AROUND_STATEMENTS_CHECK_H

clang-tidy/misc/BracesAroundStatementsCheck.cpp

for (;;) {
     // Fast-forward current token.
     Loc = TokEndLoc;
   }
   return Loc;
 }
 
 } // namespace
 
+BracesAroundStatementsCheck::BracesAroundStatementsCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      // Always add braces by default.
+      ShortStatementLines(Options.get("ShortStatementLines", 0U)) {}
+
+void
+BracesAroundStatementsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "ShortStatementLines", ShortStatementLines);
+}
+
 void BracesAroundStatementsCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(ifStmt().bind("if"), this);
   Finder->addMatcher(whileStmt().bind("while"), this);
   Finder->addMatcher(doStmt().bind("do"), this);
   Finder->addMatcher(forStmt().bind("for"), this);
   Finder->addMatcher(forRangeStmt().bind("for-range"), this);
 }
 
BracesAroundStatementsCheck::checkStmt(const MatchFinder::MatchResult &Result,
   if (!S || isa<CompoundStmt>(S)) {
     // Already inside braces.
     return;
   }
   // Skip macros.
   if (S->getLocStart().isMacroID())
     return;
 
-  // TODO: Add an option to insert braces if:
-  //   * the body doesn't fit on the same line with the control statement
-  //   * the body takes more than one line
-  //   * always.
-
   const SourceManager &SM = *Result.SourceManager;
   const ASTContext *Context = Result.Context;
 
   // InitialLoc points at the last token before opening brace to be inserted.
   assert(InitialLoc.isValid());
   SourceLocation StartLoc =
       Lexer::getLocForEndOfToken(InitialLoc, 0, SM, Context->getLangOpts());
   // StartLoc points at the location of the opening brace to be inserted.
   SourceLocation EndLoc;
   std::string ClosingInsertion;
   if (EndLocHint.isValid()) {
     EndLoc = EndLocHint;
     ClosingInsertion = "} ";
   } else {
     EndLoc = findEndLocation(S->getLocEnd(), SM, Context);
     ClosingInsertion = "\n}";
   }
 
   assert(StartLoc.isValid());
+  assert(EndLoc.isValid());
+  // Don't require braces for statements spanning less than certain number of
+  // lines.
+  if (ShortStatementLines) {
+    unsigned StartLine = SM.getSpellingLineNumber(StartLoc);
+    unsigned EndLine = SM.getSpellingLineNumber(EndLoc);
+    if (EndLine - StartLine < ShortStatementLines)
+      return;
+  }
+
   auto Diag = diag(StartLoc, "statement should be inside braces");
   Diag << FixItHint::CreateInsertion(StartLoc, " {")
        << FixItHint::CreateInsertion(EndLoc, ClosingInsertion);
 }
 
 } // namespace tidy
 } // namespace clang

test/clang-tidy/misc-braces-around-statements-always.cpp

+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-braces-around-statements %t -config="{CheckOptions: [{key: misc-braces-around-statements.ShortStatementLines, value: 0}]}" --
+// REQUIRES: shell
+
+void do_something(const char *) {}
+
+bool cond(const char *) {
+  return false;
+}
+
+void test() {
+  if (cond("if1") /*comment*/) do_something("same-line");
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: statement should be inside braces
+  // CHECK-FIXES:   if (cond("if1") /*comment*/) { do_something("same-line");
+  // CHECK-FIXES: }
+
+  if (cond("if2"))
+    do_something("single-line");
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning: statement should be inside braces
+  // CHECK-FIXES: if (cond("if2")) {
+  // CHECK-FIXES: }
+
+  if (cond("if3") /*comment*/)
+    // some comment
+    do_something("three"
+                 "lines");
+  // CHECK-MESSAGES: :[[@LINE-4]]:31: warning: statement should be inside braces
+  // CHECK-FIXES: if (cond("if3") /*comment*/) {
+  // CHECK-FIXES: }
+
+  if (cond("if4") /*comment*/)
+    // some comment
+    do_something("many"
+                 "many"
+                 "many"
+                 "many"
+                 "lines");
+  // CHECK-MESSAGES: :[[@LINE-7]]:31: warning: statement should be inside braces
+  // CHECK-FIXES: if (cond("if4") /*comment*/) {
+  // CHECK-FIXES: }
+
+  // CHECK-NOT: warning
+}

test/clang-tidy/misc-braces-around-statements-few-lines.cpp

+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-braces-around-statements %t -config="{CheckOptions: [{key: misc-braces-around-statements.ShortStatementLines, value: 4}]}" --
+// REQUIRES: shell
+
+void do_something(const char *) {}
+
+bool cond(const char *) {
+  return false;
+}
+
+void test() {
+  if (cond("if1") /*comment*/) do_something("same-line");
+  // CHECK-NOT: warning
+
+  if (cond("if2"))
+    do_something("single-line");
+  // CHECK-NOT: warning
+
+  if (cond("if3") /*comment*/)
+    // some comment
+    do_something("three"
+                 "lines");
+  // CHECK-NOT: warning
+
+  if (cond("if4") /*comment*/)
+    // some comment
+    do_something("many"
+                 "many"
+                 "many"
+                 "many"
+                 "lines");
+  // CHECK-MESSAGES: :[[@LINE-7]]:31: warning: statement should be inside braces
+  // CHECK-FIXES: if (cond("if4") /*comment*/) {
+  // CHECK-FIXES: }
+
+  // CHECK-NOT: warning
+}

test/clang-tidy/misc-braces-around-statements-same-line.cpp

+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-braces-around-statements %t -config="{CheckOptions: [{key: misc-braces-around-statements.ShortStatementLines, value: 1}]}" --
+// REQUIRES: shell
+
+void do_something(const char *) {}
+
+bool cond(const char *) {
+  return false;
+}
+
+void test() {
+  if (cond("if1") /*comment*/) do_something("same-line");
+  // CHECK-NOT: warning
+
+  if (cond("if2"))
+    do_something("single-line");
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning: statement should be inside braces
+  // CHECK-FIXES: if (cond("if2")) {
+  // CHECK-FIXES: }
+
+  if (cond("if3") /*comment*/)
+    // some comment
+    do_something("three"
+                 "lines");
+  // CHECK-MESSAGES: :[[@LINE-4]]:31: warning: statement should be inside braces
+  // CHECK-FIXES: if (cond("if3") /*comment*/) {
+  // CHECK-FIXES: }
+
+  if (cond("if4") /*comment*/)
+    // some comment
+    do_something("many"
+                 "many"
+                 "many"
+                 "many"
+                 "lines");
+  // CHECK-MESSAGES: :[[@LINE-7]]:31: warning: statement should be inside braces
+  // CHECK-FIXES: if (cond("if4") /*comment*/) {
+  // CHECK-FIXES: }
+
+  // CHECK-NOT: warning
+}

test/clang-tidy/misc-braces-around-statements-single-line.cpp

+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-braces-around-statements %t -config="{CheckOptions: [{key: misc-braces-around-statements.ShortStatementLines, value: 2}]}" --
+// REQUIRES: shell
+
+void do_something(const char *) {}
+
+bool cond(const char *) {
+  return false;
+}
+
+void test() {
+  if (cond("if1") /*comment*/) do_something("same-line");
+  // CHECK-NOT: warning
+
+  if (cond("if2"))
+    do_something("single-line");
+  // CHECK-NOT: warning
+
+  if (cond("if3") /*comment*/)
+    // some comment
+    do_something("three"
+                 "lines");
+  // CHECK-MESSAGES: :[[@LINE-4]]:31: warning: statement should be inside braces
+  // CHECK-FIXES: if (cond("if3") /*comment*/) {
+  // CHECK-FIXES: }
+
+  if (cond("if4") /*comment*/)
+    // some comment
+    do_something("many"
+                 "many"
+                 "many"
+                 "many"
+                 "lines");
+  // CHECK-MESSAGES: :[[@LINE-7]]:31: warning: statement should be inside braces
+  // CHECK-FIXES: if (cond("if4") /*comment*/) {
+  // CHECK-FIXES: }
+
+  // CHECK-NOT: warning
+}