From 45cd405dc07bbc259ea251c8f5f5e2bca7a6112c Mon Sep 17 00:00:00 2001 From: Diana Picus Date: Fri, 11 Jun 2021 09:07:23 +0000 Subject: [PATCH] [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 --- flang/.clang-tidy | 2 +- flang/docs/C++style.md | 5 ++- flang/include/flang/Lower/.clang-tidy | 2 +- flang/include/flang/Optimizer/.clang-tidy | 2 +- flang/lib/Lower/.clang-tidy | 2 +- flang/lib/Optimizer/.clang-tidy | 2 +- flang/lib/Semantics/canonicalize-acc.cpp | 6 ++- flang/lib/Semantics/check-acc-structure.cpp | 18 ++++++--- flang/lib/Semantics/check-omp-structure.cpp | 9 +++-- flang/lib/Semantics/resolve-directives.cpp | 38 ++++++++++++------- flang/runtime/ISO_Fortran_binding.cpp | 3 +- flang/tools/.clang-tidy | 2 + .../RuntimeGTest/CrashHandlerFixture.cpp | 5 ++- 13 files changed, 64 insertions(+), 32 deletions(-) create mode 100644 flang/tools/.clang-tidy diff --git a/flang/.clang-tidy b/flang/.clang-tidy index 7eb4d259fba3..ee3a0ab2201b 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 fb11e6411614..16d0b1bc4744 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 934c21cd6772..9a0c8a6d4cbf 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 934c21cd6772..9a0c8a6d4cbf 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 934c21cd6772..9a0c8a6d4cbf 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 934c21cd6772..9a0c8a6d4cbf 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 0241508543ba..855f62f53ff8 100644 --- a/flang/lib/Semantics/canonicalize-acc.cpp +++ b/flang/lib/Semantics/canonicalize-acc.cpp @@ -64,8 +64,9 @@ private: std::size_t tileArgNb = listTileExpr.size(); const auto &outer{std::get>(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(loop->t)}; @@ -90,8 +91,9 @@ private: template void CheckDoConcurrentClauseRestriction(const C &x) { const auto &doCons{std::get>(x.t)}; - if (!doCons->IsDoConcurrent()) + if (!doCons->IsDoConcurrent()) { return; + } const auto &beginLoopDirective = std::get(x.t); const auto &accClauseList = std::get(beginLoopDirective.t); diff --git a/flang/lib/Semantics/check-acc-structure.cpp b/flang/lib/Semantics/check-acc-structure.cpp index 537b59d925ae..2cb1059a57b3 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>(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>(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 07ca47d5c43b..1ff292267190 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(loop->t)}; @@ -782,8 +784,9 @@ void OmpStructureChecker::Enter(const parser::OpenMPExecutableAllocate &x) { const auto &dir{std::get(x.t)}; const auto &objectList{std::get>(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 70fa51b28668..67e2bd4f812b 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>(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(&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>(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(x.t)}; for (const auto &clause : clauseList.v) { - if (std::get_if(&clause.u)) + if (std::get_if(&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()}) + if (const auto *details{symbol.detailsIf()}) { 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 492d383b19bc..c49bb6761686 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 000000000000..84b3c0fe3ea5 --- /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 7078949d0132..355ae8f3703b 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; }