[flang-commits] [flang] 45cd405 - [flang] Add clang-tidy check for braces around if
Diana Picus via flang-commits
flang-commits at lists.llvm.org
Wed Jun 16 02:14:07 PDT 2021
Author: Diana Picus
Date: 2021-06-16T09:13:53Z
New Revision: 45cd405dc07bbc259ea251c8f5f5e2bca7a6112c
URL: https://github.com/llvm/llvm-project/commit/45cd405dc07bbc259ea251c8f5f5e2bca7a6112c
DIFF: https://github.com/llvm/llvm-project/commit/45cd405dc07bbc259ea251c8f5f5e2bca7a6112c.diff
LOG: [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
Added:
flang/tools/.clang-tidy
Modified:
flang/.clang-tidy
flang/docs/C++style.md
flang/include/flang/Lower/.clang-tidy
flang/include/flang/Optimizer/.clang-tidy
flang/lib/Lower/.clang-tidy
flang/lib/Optimizer/.clang-tidy
flang/lib/Semantics/canonicalize-acc.cpp
flang/lib/Semantics/check-acc-structure.cpp
flang/lib/Semantics/check-omp-structure.cpp
flang/lib/Semantics/resolve-directives.cpp
flang/runtime/ISO_Fortran_binding.cpp
flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
Removed:
################################################################################
diff --git a/flang/.clang-tidy b/flang/.clang-tidy
index 7eb4d259fba39..ee3a0ab2201bf 100644
--- a/flang/.clang-tidy
+++ b/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
diff --git a/flang/docs/C++style.md b/flang/docs/C++style.md
index fb11e64116141..16d0b1bc47442 100644
--- a/flang/docs/C++style.md
+++ b/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
diff --git a/flang/include/flang/Lower/.clang-tidy b/flang/include/flang/Lower/.clang-tidy
index 934c21cd67722..9a0c8a6d4cbfb 100644
--- a/flang/include/flang/Lower/.clang-tidy
+++ b/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
diff --git a/flang/include/flang/Optimizer/.clang-tidy b/flang/include/flang/Optimizer/.clang-tidy
index 934c21cd67722..9a0c8a6d4cbfb 100644
--- a/flang/include/flang/Optimizer/.clang-tidy
+++ b/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
diff --git a/flang/lib/Lower/.clang-tidy b/flang/lib/Lower/.clang-tidy
index 934c21cd67722..9a0c8a6d4cbfb 100644
--- a/flang/lib/Lower/.clang-tidy
+++ b/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
diff --git a/flang/lib/Optimizer/.clang-tidy b/flang/lib/Optimizer/.clang-tidy
index 934c21cd67722..9a0c8a6d4cbfb 100644
--- a/flang/lib/Optimizer/.clang-tidy
+++ b/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
diff --git a/flang/lib/Semantics/canonicalize-acc.cpp b/flang/lib/Semantics/canonicalize-acc.cpp
index 0241508543ba8..855f62f53ff8d 100644
--- a/flang/lib/Semantics/canonicalize-acc.cpp
+++ b/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);
diff --git a/flang/lib/Semantics/check-acc-structure.cpp b/flang/lib/Semantics/check-acc-structure.cpp
index 537b59d925aeb..2cb1059a57b35 100644
--- a/flang/lib/Semantics/check-acc-structure.cpp
+++ b/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,7 +151,7 @@ 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 "
@@ -156,6 +159,7 @@ void AccStructureChecker::Leave(
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 "
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 07ca47d5c43b9..1ff2922671902 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/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) {
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 70fa51b286688..67e2bd4f812b3 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/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,7 +780,7 @@ 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,
@@ -785,6 +788,7 @@ void AccAttributeVisitor::DoNotAllowAssumedSizedArray(
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,
diff --git a/flang/runtime/ISO_Fortran_binding.cpp b/flang/runtime/ISO_Fortran_binding.cpp
index 492d383b19bc2..c49bb67616869 100644
--- a/flang/runtime/ISO_Fortran_binding.cpp
+++ b/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");
diff --git a/flang/tools/.clang-tidy b/flang/tools/.clang-tidy
new file mode 100644
index 0000000000000..84b3c0fe3ea59
--- /dev/null
+++ b/flang/tools/.clang-tidy
@@ -0,0 +1,2 @@
+Checks: '-readability-braces-around-statements'
+InheritParentConfig: true
diff --git a/flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp b/flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
index 7078949d0132f..355ae8f3703bf 100644
--- a/flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
+++ b/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;
}
More information about the flang-commits
mailing list