[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
GitOrigin-RevId: 45cd405dc07bbc259ea251c8f5f5e2bca7a6112c
diff --git a/.clang-tidy b/.clang-tidy
index 7eb4d25..ee3a0ab 100644
--- a/.clang-tidy
+++ b/.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
diff --git a/docs/C++style.md b/docs/C++style.md
index fb11e64..16d0b1b 100644
--- a/docs/C++style.md
+++ b/docs/C++style.md
@@ -115,7 +115,10 @@
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
diff --git a/include/flang/Lower/.clang-tidy b/include/flang/Lower/.clang-tidy
index 934c21c..9a0c8a6 100644
--- a/include/flang/Lower/.clang-tidy
+++ b/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
diff --git a/include/flang/Optimizer/.clang-tidy b/include/flang/Optimizer/.clang-tidy
index 934c21c..9a0c8a6 100644
--- a/include/flang/Optimizer/.clang-tidy
+++ b/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
diff --git a/lib/Lower/.clang-tidy b/lib/Lower/.clang-tidy
index 934c21c..9a0c8a6 100644
--- a/lib/Lower/.clang-tidy
+++ b/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
diff --git a/lib/Optimizer/.clang-tidy b/lib/Optimizer/.clang-tidy
index 934c21c..9a0c8a6 100644
--- a/lib/Optimizer/.clang-tidy
+++ b/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
diff --git a/lib/Semantics/canonicalize-acc.cpp b/lib/Semantics/canonicalize-acc.cpp
index 0241508..855f62f 100644
--- a/lib/Semantics/canonicalize-acc.cpp
+++ b/lib/Semantics/canonicalize-acc.cpp
@@ -64,8 +64,9 @@
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 @@
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);
diff --git a/lib/Semantics/check-acc-structure.cpp b/lib/Semantics/check-acc-structure.cpp
index 537b59d..2cb1059 100644
--- a/lib/Semantics/check-acc-structure.cpp
+++ b/lib/Semantics/check-acc-structure.cpp
@@ -65,22 +65,25 @@
}
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,7 +151,7 @@
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 "
@@ -156,6 +159,7 @@
parser::ToUpperCaseLetters(
llvm::acc::getOpenACCClauseName(cl).str()),
ContextDirectiveAsFortran());
+ }
}
}
dirContext_.pop_back();
@@ -368,8 +372,9 @@
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 @@
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 "
diff --git a/lib/Semantics/check-omp-structure.cpp b/lib/Semantics/check-omp-structure.cpp
index 07ca47d..1ff2922 100644
--- a/lib/Semantics/check-omp-structure.cpp
+++ b/lib/Semantics/check-omp-structure.cpp
@@ -545,8 +545,9 @@
// 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 @@
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 @@
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) {
diff --git a/lib/Semantics/resolve-directives.cpp b/lib/Semantics/resolve-directives.cpp
index 70fa51b..67e2bd4 100644
--- a/lib/Semantics/resolve-directives.cpp
+++ b/lib/Semantics/resolve-directives.cpp
@@ -694,20 +694,22 @@
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 @@
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,7 +780,7 @@
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,
@@ -785,6 +788,7 @@
llvm::acc::getOpenACCDirectiveName(
GetContext().directive)
.str()));
+ }
},
[&](const auto &name) {
@@ -861,13 +865,14 @@
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::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 @@
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 @@
// 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 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 @@
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,
diff --git a/runtime/ISO_Fortran_binding.cpp b/runtime/ISO_Fortran_binding.cpp
index 492d383..c49bb67 100644
--- a/runtime/ISO_Fortran_binding.cpp
+++ b/runtime/ISO_Fortran_binding.cpp
@@ -236,8 +236,9 @@
}
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");
diff --git a/tools/.clang-tidy b/tools/.clang-tidy
new file mode 100644
index 0000000..84b3c0f
--- /dev/null
+++ b/tools/.clang-tidy
@@ -0,0 +1,2 @@
+Checks: '-readability-braces-around-statements'
+InheritParentConfig: true
diff --git a/unittests/RuntimeGTest/CrashHandlerFixture.cpp b/unittests/RuntimeGTest/CrashHandlerFixture.cpp
index 7078949..355ae8f 100644
--- a/unittests/RuntimeGTest/CrashHandlerFixture.cpp
+++ b/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;
}
