[flang] Add clang-tidy check for braces around if

Flang diverges from the llvm coding style in that it requires braces
around the bodies of if/while/etc statements, even when the body is
a single statement.

This commit adds the readability-braces-around-statements check to
flang's clang-tidy config file. Hopefully the premerge bots will pick it
up and report violations in Phabricator.

We also explicitly disable the check in the directories corresponding to
the Lower and Optimizer libraries, which rely heavily on mlir and llvm
and therefore follow their coding style. Likewise for the tools
directory.

We also fix any outstanding violations in the runtime and in
lib/Semantics.

Differential Revision: https://reviews.llvm.org/D104100

Author: rovka

flang/.clang-tidy

@@ -1,2 +1,2 @@
-Checks: '-llvm-include-order,-readability-identifier-naming,-clang-diagnostic-*'
+Checks: '-llvm-include-order,readability-braces-around-statements,-readability-identifier-naming,-clang-diagnostic-*'
 InheritParentConfig: true

flang/docs/C++style.md

@@ -115,7 +115,10 @@ Don't try to make columns of variable names or comments
 align vertically -- they are maintenance problems.
 
 Always wrap the bodies of `if()`, `else`, `while()`, `for()`, `do`, &c.
-with braces, even when the body is a single statement or empty.  The
+with braces, even when the body is a single statement or empty.  Note that this
+diverges from the LLVM coding style.  In parts of the codebase that make heavy
+use of LLVM or MLIR APIs (e.g. the Lower and Optimizer libraries), use the
+LLVM style instead.  The
 opening `{` goes on
 the end of the line, not on the next line.  Functions also put the opening
 `{` after the formal arguments or new-style result type, not on the next

flang/include/flang/Lower/.clang-tidy

@@ -1,4 +1,4 @@
-Checks: 'readability-identifier-naming,llvm-include-order,clang-diagnostic-*'
+Checks: '-readability-braces-around-statements,readability-identifier-naming,llvm-include-order,clang-diagnostic-*'
 InheritParentConfig: true
 CheckOptions:
   - key:             readability-identifier-naming.MemberCase

flang/include/flang/Optimizer/.clang-tidy

@@ -1,4 +1,4 @@
-Checks: 'readability-identifier-naming,llvm-include-order,clang-diagnostic-*'
+Checks: '-readability-braces-around-statements,readability-identifier-naming,llvm-include-order,clang-diagnostic-*'
 InheritParentConfig: true
 CheckOptions:
   - key:             readability-identifier-naming.MemberCase

flang/lib/Lower/.clang-tidy

@@ -1,4 +1,4 @@
-Checks: 'readability-identifier-naming,llvm-include-order,clang-diagnostic-*'
+Checks: '-readability-braces-around-statements,readability-identifier-naming,llvm-include-order,clang-diagnostic-*'
 InheritParentConfig: true
 CheckOptions:
   - key:             readability-identifier-naming.MemberCase

flang/lib/Optimizer/.clang-tidy

@@ -1,4 +1,4 @@
-Checks: 'readability-identifier-naming,llvm-include-order,clang-diagnostic-*'
+Checks: '-readability-braces-around-statements,readability-identifier-naming,llvm-include-order,clang-diagnostic-*'
 InheritParentConfig: true
 CheckOptions:
   - key:             readability-identifier-naming.MemberCase

flang/lib/Semantics/canonicalize-acc.cpp

@@ -64,8 +64,9 @@ class CanonicalizationOfAcc {
         std::size_t tileArgNb = listTileExpr.size();
 
         const auto &outer{std::get<std::optional<parser::DoConstruct>>(x.t)};
-        if (outer->IsDoConcurrent())
+        if (outer->IsDoConcurrent()) {
           return; // Tile is not allowed on DO CONURRENT
+        }
         for (const parser::DoConstruct *loop{&*outer}; loop && tileArgNb > 0;
              --tileArgNb) {
           const auto &block{std::get<parser::Block>(loop->t)};
@@ -90,8 +91,9 @@ class CanonicalizationOfAcc {
   template <typename C, typename D>
   void CheckDoConcurrentClauseRestriction(const C &x) {
     const auto &doCons{std::get<std::optional<parser::DoConstruct>>(x.t)};
-    if (!doCons->IsDoConcurrent())
+    if (!doCons->IsDoConcurrent()) {
       return;
+    }
     const auto &beginLoopDirective = std::get<D>(x.t);
     const auto &accClauseList =
         std::get<parser::AccClauseList>(beginLoopDirective.t);

flang/lib/Semantics/check-acc-structure.cpp

@@ -65,22 +65,25 @@ bool AccStructureChecker::IsComputeConstruct(
 }
 
 bool AccStructureChecker::IsInsideComputeConstruct() const {
-  if (dirContext_.size() <= 1)
+  if (dirContext_.size() <= 1) {
     return false;
+  }
 
   // Check all nested context skipping the first one.
   for (std::size_t i = dirContext_.size() - 1; i > 0; --i) {
-    if (IsComputeConstruct(dirContext_[i - 1].directive))
+    if (IsComputeConstruct(dirContext_[i - 1].directive)) {
       return true;
+    }
   }
   return false;
 }
 
 void AccStructureChecker::CheckNotInComputeConstruct() {
-  if (IsInsideComputeConstruct())
+  if (IsInsideComputeConstruct()) {
     context_.Say(GetContext().directiveSource,
         "Directive %s may not be called within a compute region"_err_en_US,
         ContextDirectiveAsFortran());
+  }
 }
 
 void AccStructureChecker::Enter(const parser::AccClause &x) {
@@ -148,14 +151,15 @@ void AccStructureChecker::Leave(
       if (cl != llvm::acc::Clause::ACCC_create &&
           cl != llvm::acc::Clause::ACCC_copyin &&
           cl != llvm::acc::Clause::ACCC_device_resident &&
-          cl != llvm::acc::Clause::ACCC_link)
+          cl != llvm::acc::Clause::ACCC_link) {
         context_.Say(GetContext().directiveSource,
             "%s clause is not allowed on the %s directive in module "
             "declaration "
             "section"_err_en_US,
             parser::ToUpperCaseLetters(
                 llvm::acc::getOpenACCClauseName(cl).str()),
             ContextDirectiveAsFortran());
+      }
     }
   }
   dirContext_.pop_back();
@@ -368,8 +372,9 @@ void AccStructureChecker::Enter(const parser::AccClause::Copyin &c) {
   const auto &modifierClause{c.v};
   if (const auto &modifier{
           std::get<std::optional<parser::AccDataModifier>>(modifierClause.t)}) {
-    if (CheckAllowedModifier(llvm::acc::Clause::ACCC_copyin))
+    if (CheckAllowedModifier(llvm::acc::Clause::ACCC_copyin)) {
       return;
+    }
     if (modifier->v != parser::AccDataModifier::Modifier::ReadOnly) {
       context_.Say(GetContext().clauseSource,
           "Only the READONLY modifier is allowed for the %s clause "
@@ -387,8 +392,9 @@ void AccStructureChecker::Enter(const parser::AccClause::Copyout &c) {
   const auto &modifierClause{c.v};
   if (const auto &modifier{
           std::get<std::optional<parser::AccDataModifier>>(modifierClause.t)}) {
-    if (CheckAllowedModifier(llvm::acc::Clause::ACCC_copyout))
+    if (CheckAllowedModifier(llvm::acc::Clause::ACCC_copyout)) {
       return;
+    }
     if (modifier->v != parser::AccDataModifier::Modifier::Zero) {
       context_.Say(GetContext().clauseSource,
           "Only the ZERO modifier is allowed for the %s clause "

flang/lib/Semantics/check-omp-structure.cpp

@@ -545,8 +545,9 @@ void OmpStructureChecker::CheckDistLinear(
     // Get collapse level, if given, to find which loops are "associated."
     std::int64_t collapseVal{GetOrdCollapseLevel(x)};
     // Include the top loop if no collapse is specified
-    if (collapseVal == 0)
+    if (collapseVal == 0) {
       collapseVal = 1;
+    }
 
     // Match the loop index variables with the collected symbols from linear
     // clauses.
@@ -560,8 +561,9 @@ void OmpStructureChecker::CheckDistLinear(
             indexVars.erase(*(itrVal.symbol));
           }
           collapseVal--;
-          if (collapseVal == 0)
+          if (collapseVal == 0) {
             break;
+          }
         }
         // Get the next DoConstruct if block is not empty.
         const auto &block{std::get<parser::Block>(loop->t)};
@@ -782,8 +784,9 @@ void OmpStructureChecker::Enter(const parser::OpenMPExecutableAllocate &x) {
   const auto &dir{std::get<parser::Verbatim>(x.t)};
   const auto &objectList{std::get<std::optional<parser::OmpObjectList>>(x.t)};
   PushContextAndClauseSets(dir.source, llvm::omp::Directive::OMPD_allocate);
-  if (objectList)
+  if (objectList) {
     CheckIsVarPartOfAnotherVar(dir.source, *objectList);
+  }
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPExecutableAllocate &x) {

flang/lib/Semantics/resolve-directives.cpp

@@ -694,20 +694,22 @@ Symbol *AccAttributeVisitor::ResolveName(const parser::Name &name) {
 bool AccAttributeVisitor::Pre(const parser::OpenACCRoutineConstruct &x) {
   const auto &optName{std::get<std::optional<parser::Name>>(x.t)};
   if (optName) {
-    if (!ResolveName(*optName))
+    if (!ResolveName(*optName)) {
       context_.Say((*optName).source,
           "No function or subroutine declared for '%s'"_err_en_US,
           (*optName).source);
+    }
   }
   return true;
 }
 
 bool AccAttributeVisitor::Pre(const parser::AccBindClause &x) {
   if (const auto *name{std::get_if<parser::Name>(&x.u)}) {
-    if (!ResolveName(*name))
+    if (!ResolveName(*name)) {
       context_.Say(name->source,
           "No function or subroutine declared for '%s'"_err_en_US,
           name->source);
+    }
   }
   return true;
 }
@@ -750,13 +752,14 @@ void AccAttributeVisitor::AllowOnlyArrayAndSubArray(
     std::visit(
         common::visitors{
             [&](const parser::Designator &designator) {
-              if (!IsLastNameArray(designator))
+              if (!IsLastNameArray(designator)) {
                 context_.Say(designator.source,
                     "Only array element or subarray are allowed in %s directive"_err_en_US,
                     parser::ToUpperCaseLetters(
                         llvm::acc::getOpenACCDirectiveName(
                             GetContext().directive)
                             .str()));
+              }
             },
             [&](const auto &name) {
               context_.Say(name.source,
@@ -777,14 +780,15 @@ void AccAttributeVisitor::DoNotAllowAssumedSizedArray(
         common::visitors{
             [&](const parser::Designator &designator) {
               const auto &name{GetLastName(designator)};
-              if (name.symbol && semantics::IsAssumedSizeArray(*name.symbol))
+              if (name.symbol && semantics::IsAssumedSizeArray(*name.symbol)) {
                 context_.Say(designator.source,
                     "Assumed-size dummy arrays may not appear on the %s "
                     "directive"_err_en_US,
                     parser::ToUpperCaseLetters(
                         llvm::acc::getOpenACCDirectiveName(
                             GetContext().directive)
                             .str()));
+              }
             },
             [&](const auto &name) {
 
@@ -861,13 +865,14 @@ void AccAttributeVisitor::EnsureAllocatableOrPointer(
         common::visitors{
             [&](const parser::Designator &designator) {
               const auto &lastName{GetLastName(designator)};
-              if (!IsAllocatableOrPointer(*lastName.symbol))
+              if (!IsAllocatableOrPointer(*lastName.symbol)) {
                 context_.Say(designator.source,
                     "Argument `%s` on the %s clause must be a variable or "
                     "array with the POINTER or ALLOCATABLE attribute"_err_en_US,
                     lastName.symbol->name(),
                     parser::ToUpperCaseLetters(
                         llvm::acc::getOpenACCClauseName(clause).str()));
+              }
             },
             [&](const auto &name) {
               context_.Say(name.source,
@@ -1349,8 +1354,9 @@ bool OmpAttributeVisitor::Pre(const parser::OpenMPDeclarativeAllocate &x) {
 bool OmpAttributeVisitor::Pre(const parser::OpenMPExecutableAllocate &x) {
   PushContext(x.source, llvm::omp::Directive::OMPD_allocate);
   const auto &list{std::get<std::optional<parser::OmpObjectList>>(x.t)};
-  if (list)
+  if (list) {
     ResolveOmpObjectList(*list, Symbol::Flag::OmpExecutableAllocateDirective);
+  }
   return true;
 }
 
@@ -1376,8 +1382,9 @@ void OmpAttributeVisitor::Post(const parser::OmpDefaultClause &x) {
 bool OmpAttributeVisitor::IsNestedInDirective(llvm::omp::Directive directive) {
   if (dirContext_.size() >= 1) {
     for (std::size_t i = dirContext_.size() - 1; i > 0; --i) {
-      if (dirContext_[i - 1].directive == directive)
+      if (dirContext_[i - 1].directive == directive) {
         return true;
+      }
     }
   }
   return false;
@@ -1389,17 +1396,19 @@ void OmpAttributeVisitor::Post(const parser::OpenMPExecutableAllocate &x) {
   // parser::Unwrap instead of the following loop
   const auto &clauseList{std::get<parser::OmpClauseList>(x.t)};
   for (const auto &clause : clauseList.v) {
-    if (std::get_if<parser::OmpClause::Allocator>(&clause.u))
+    if (std::get_if<parser::OmpClause::Allocator>(&clause.u)) {
       hasAllocator = true;
+    }
   }
 
-  if (IsNestedInDirective(llvm::omp::Directive::OMPD_target) && !hasAllocator)
+  if (IsNestedInDirective(llvm::omp::Directive::OMPD_target) && !hasAllocator) {
     // TODO: expand this check to exclude the case when a requires
     //       directive with the dynamic_allocators clause is present
     //       in the same compilation unit (OMP5.0 2.11.3).
     context_.Say(x.source,
         "ALLOCATE directives that appear in a TARGET region "
         "must specify an allocator clause"_err_en_US);
+  }
   PopContext();
 }
 
@@ -1675,16 +1684,18 @@ void ResolveOmpParts(
 void OmpAttributeVisitor::CheckDataCopyingClause(
     const parser::Name &name, const Symbol &symbol, Symbol::Flag ompFlag) {
   const auto *checkSymbol{&symbol};
-  if (const auto *details{symbol.detailsIf<HostAssocDetails>()})
+  if (const auto *details{symbol.detailsIf<HostAssocDetails>()}) {
     checkSymbol = &details->symbol();
+  }
 
   if (ompFlag == Symbol::Flag::OmpCopyIn) {
     // List of items/objects that can appear in a 'copyin' clause must be
     // 'threadprivate'
-    if (!checkSymbol->test(Symbol::Flag::OmpThreadprivate))
+    if (!checkSymbol->test(Symbol::Flag::OmpThreadprivate)) {
       context_.Say(name.source,
           "Non-THREADPRIVATE object '%s' in COPYIN clause"_err_en_US,
           checkSymbol->name());
+    }
   } else if (ompFlag == Symbol::Flag::OmpCopyPrivate &&
       GetContext().directive == llvm::omp::Directive::OMPD_single) {
     // A list item that appears in a 'copyprivate' clause may not appear on a
@@ -1715,10 +1726,11 @@ void OmpAttributeVisitor::CheckPrivateDSAObject(
     const parser::Name &name, const Symbol &symbol, Symbol::Flag ompFlag) {
   const auto &ultimateSymbol{symbol.GetUltimate()};
   llvm::StringRef clauseName{"PRIVATE"};
-  if (ompFlag == Symbol::Flag::OmpFirstPrivate)
+  if (ompFlag == Symbol::Flag::OmpFirstPrivate) {
     clauseName = "FIRSTPRIVATE";
-  else if (ompFlag == Symbol::Flag::OmpLastPrivate)
+  } else if (ompFlag == Symbol::Flag::OmpLastPrivate) {
     clauseName = "LASTPRIVATE";
+  }
 
   if (ultimateSymbol.test(Symbol::Flag::InNamelist)) {
     context_.Say(name.source,

flang/runtime/ISO_Fortran_binding.cpp

@@ -236,8 +236,9 @@ int CFI_establish(CFI_cdesc_t *descriptor, void *base_addr,
   }
   if (type == CFI_type_struct || type == CFI_type_other ||
       IsCharacterType(type)) {
-    if (elem_len <= 0)
+    if (elem_len <= 0) {
       return CFI_INVALID_ELEM_LEN;
+    }
   } else {
     elem_len = MinElemLen(type);
     assert(elem_len > 0 && "Unknown element length for type");

flang/tools/.clang-tidy

@@ -0,0 +1,2 @@
+Checks: '-readability-braces-around-statements'
+InheritParentConfig: true

flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp

@@ -28,7 +28,10 @@
 // Register the crash handler above when creating each unit test in this suite
 void CrashHandlerFixture::SetUp() {
   static bool isCrashHanlderRegistered{false};
-  if (!isCrashHanlderRegistered)
+
+  if (!isCrashHanlderRegistered) {
     Fortran::runtime::Terminator::RegisterCrashHandler(CatchCrash);
+  }
+
   isCrashHanlderRegistered = true;
 }