Force braces on the else branch if they are being added to the if branch.

[sbenza]

Summary

Force braces on the else branch if they are being added to the if branch.
This ensures consistency in the transformed code.

[alexfh]

Awesome! Thanks for the fix!

clang-tidy/readability/BracesAroundStatementsCheck.h

46 ↗

(On Diff #22905)

Maybe add a comment describing the meaning of the return value?

[sbenza]

Added comment

clang-tools-extra/trunk/clang-tidy/readability/BracesAroundStatementsCheck.h

 class BracesAroundStatementsCheck : public ClangTidyCheck {
 public:
   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,
+  bool checkStmt(const ast_matchers::MatchFinder::MatchResult &Result,
                  const Stmt *S, SourceLocation StartLoc,
-                 SourceLocation EndLocHint = SourceLocation());
+                 SourceLocation EndLocHint = SourceLocation(),
+                 bool ForceBraces = false);
   template <typename IfOrWhileStmt>
   SourceLocation findRParenLoc(const IfOrWhileStmt *S, const SourceManager &SM,
                                const ASTContext *Context);
 
 private:
   const unsigned ShortStatementLines;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_BRACESAROUNDSTATEMENTSCHECK_H

clang-tools-extra/trunk/clang-tidy/readability/BracesAroundStatementsCheck.cpp

if (auto S = Result.Nodes.getNodeAs<ForStmt>("for")) {
     SourceLocation StartLoc = findRParenLoc(S, SM, Context);
     if (StartLoc.isInvalid())
       return;
     checkStmt(Result, S->getBody(), StartLoc);
   } else if (auto S = Result.Nodes.getNodeAs<IfStmt>("if")) {
     SourceLocation StartLoc = findRParenLoc(S, SM, Context);
     if (StartLoc.isInvalid())
       return;
-    checkStmt(Result, S->getThen(), StartLoc, S->getElseLoc());
+    bool BracedIf = checkStmt(Result, S->getThen(), StartLoc, S->getElseLoc());
     const Stmt *Else = S->getElse();
     if (Else && !isa<IfStmt>(Else)) {
       // Omit 'else if' statements here, they will be handled directly.
-      checkStmt(Result, Else, S->getElseLoc());
+      checkStmt(Result, Else, S->getElseLoc(), SourceLocation(), BracedIf);
     }
   } else {
     llvm_unreachable("Invalid match");
   }
 }
 
 /// Find location of right parenthesis closing condition
 template <typename IfOrWhileStmt>
BracesAroundStatementsCheck::findRParenLoc(const IfOrWhileStmt *S,
   tok::TokenKind TokKind = getTokenKind(RParenLoc, SM, Context);
   if (TokKind != tok::r_paren) {
     diag(RParenLoc, ErrorMessage);
     return SourceLocation();
   }
   return RParenLoc;
 }
 
-void
-BracesAroundStatementsCheck::checkStmt(const MatchFinder::MatchResult &Result,
-                                       const Stmt *S, SourceLocation InitialLoc,
-                                       SourceLocation EndLocHint) {
+/// Determine if the statement needs braces around it, and add them if it does.
+/// Returns true if braces where added.
+bool BracesAroundStatementsCheck::checkStmt(
+    const MatchFinder::MatchResult &Result, const Stmt *S,
+    SourceLocation InitialLoc, SourceLocation EndLocHint, bool ForceBraces) {
   // 1) If there's a corresponding "else" or "while", the check inserts "} "
   // right before that token.
   // 2) If there's a multi-line block comment starting on the same line after
   // the location we're inserting the closing brace at, or there's a non-comment
   // token, the check inserts "\n}" right before that token.
   // 3) Otherwise the check finds the end of line (possibly after some block or
   // line comments) and inserts "\n}" right before that EOL.
   if (!S || isa<CompoundStmt>(S)) {
     // Already inside braces.
-    return;
+    return false;
   }
   // Skip macros.
   if (S->getLocStart().isMacroID())
-    return;
+    return false;
 
   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) {
+  if (ShortStatementLines && !ForceBraces) {
     unsigned StartLine = SM.getSpellingLineNumber(StartLoc);
     unsigned EndLine = SM.getSpellingLineNumber(EndLoc);
     if (EndLine - StartLine < ShortStatementLines)
-      return;
+      return false;
   }
 
   auto Diag = diag(StartLoc, "statement should be inside braces");
   Diag << FixItHint::CreateInsertion(StartLoc, " {")
        << FixItHint::CreateInsertion(EndLoc, ClosingInsertion);
+  return true;
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h

private:
   }
 
   ClangTidyCheck &Check;
   ast_matchers::MatchFinder &Finder;
   ClangTidyContext &Context;
 };
 
 template <typename T>
-std::string runCheckOnCode(StringRef Code,
-                           std::vector<ClangTidyError> *Errors = nullptr,
-                           const Twine &Filename = "input.cc",
-                           ArrayRef<std::string> ExtraArgs = None) {
-  ClangTidyOptions Options;
+std::string
+runCheckOnCode(StringRef Code, std::vector<ClangTidyError> *Errors = nullptr,
+               ArrayRef<std::string> ExtraArgs = None,
+               const ClangTidyOptions &ExtraOptions = ClangTidyOptions()) {
+  ClangTidyOptions Options = ExtraOptions;
   Options.Checks = "*";
   ClangTidyContext Context(llvm::make_unique<DefaultOptionsProvider>(
       ClangTidyGlobalOptions(), Options));
   ClangTidyDiagnosticConsumer DiagConsumer(Context);
   T Check("test-check", &Context);
   ast_matchers::MatchFinder Finder;
   Check.registerMatchers(&Finder);
 

clang-tools-extra/trunk/unittests/clang-tidy/ReadabilityModuleTest.cpp

EXPECT_EQ("int main() {\n"
                                                         "    return -1;\n"
                                                         "  else if (1 == 2) {\n"
                                                         "    return -2;\n"
                                                         "  } else\n"
                                                         "    return -3;\n"
                                                         "}"));
 }
 
+TEST(BracesAroundStatementsCheck, IfElseWithShortStatements) {
+  ClangTidyOptions Options;
+  Options.CheckOptions["test-check.ShortStatementLines"] = "1";
+
+  EXPECT_EQ("int main() {\n"
+            "  if (true) return 1;\n"
+            "  if (false) { return -1;\n"
+            "  } else if (1 == 2) { return -2;\n"
+            "  } else { return -3;\n"
+            "}\n"
+            "}",
+            runCheckOnCode<BracesAroundStatementsCheck>(
+                "int main() {\n"
+                "  if (true) return 1;\n"
+                "  if (false) return -1;\n"
+                "  else if (1 == 2) return -2;\n"
+                "  else return -3;\n"
+                "}",
+                nullptr, "input.cc", None, Options));
+}
+
 TEST(BracesAroundStatementsCheck, For) {
   EXPECT_NO_CHANGES(BracesAroundStatementsCheck, "int main() {\n"
                                                  "  for (;;) {\n"
                                                  "    ;\n"
                                                  "  }\n"
                                                  "}");
   EXPECT_EQ("int main() {\n"
             "  for (;;) {\n"