lebedev.ri created this revision.
lebedev.ri added reviewers: JonasToth, aaron.ballman, alexfh, xazax.hun, hokein.
lebedev.ri added projects: clang-tools-extra, OpenMP.
Herald added subscribers: guansong, rnkovacs, mgorny.
Finds OpenMP directives that are allowed to contain `default` clause,
but either don't specify it, or the clause is specified but with the kind
other than `none`, and suggests to use `default(none)` clause.
Using `default(none)` clause changes the default variable visibility from
being implicitly determined, and thus forces developer to be explicit about the
desired data scoping for each variable.
--------------------------------------------------------------------------------
Since this seems to be ground-breaking (as in the first one) OpenMP check
in (upstream?) clang-tidy, there is a lot of boilerplate matchers.
I suppose eventually (some of them?) will migrate to be proper AST matchers.
I'm not happy with how `isAllowedToContainClause()` actually takes the
`enum OpenMPClauseKind` value, instead of something nicer, be it either
the actual AST class name (`OMPDefaultClause`), or the matcher
(`ompDefaultClause`)
that would normally match that node.
Same goes for `ompDefaultClause(hasKind())`, which takes the
`enum OpenMPDefaultClauseKind` value.
Depends on D57112 <https://reviews.llvm.org/D57112>.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D57113
Files:
clang-tidy/CMakeLists.txt
clang-tidy/ClangTidyForceLinker.h
clang-tidy/openmp/CMakeLists.txt
clang-tidy/openmp/OpenMPTidyModule.cpp
clang-tidy/openmp/UseDefaultNoneCheck.cpp
clang-tidy/openmp/UseDefaultNoneCheck.h
clang-tidy/plugin/CMakeLists.txt
clang-tidy/tool/CMakeLists.txt
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/openmp-use-default-none.rst
test/clang-tidy/openmp-use-default-none.cpp
Index: test/clang-tidy/openmp-use-default-none.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/openmp-use-default-none.cpp
@@ -0,0 +1,68 @@
+// RUN: %check_clang_tidy %s openmp-use-default-none %t -- -- -x c++ -fopenmp -fopenmp-version=31
+// RUN: %check_clang_tidy %s openmp-use-default-none %t -- -- -x c -fopenmp -fopenmp-version=31
+
+//----------------------------------------------------------------------------//
+// Trivial cases.
+//----------------------------------------------------------------------------//
+
+// 'for' directive can not have 'default' clause, no diagnostics.
+void t0(const int a) {
+#pragma omp for
+ for (int b = 0; b < a; b++)
+ ;
+}
+
+// 'parallel' directive can have 'default' clause, but said clause is not
+// specified, diagnosed.
+void t1() {
+#pragma omp parallel
+ ;
+ // CHECK-NOTES: :[[@LINE-2]]:9: warning: OpenMP directive 'parallel' is allowed to contain the 'default' clause, but no 'default' clause is specified. Consider specifying 'default(none)' clause.
+}
+
+// 'parallel' directive can have 'default' clause, and said clause is specified,
+// with 'none' kind, all good.
+void t2() {
+#pragma omp parallel default(none)
+ ;
+}
+
+// 'parallel' directive can have 'default' clause, and said clause is specified,
+// but with 'shared' kind, which is not 'none', diagnose.
+void t3() {
+#pragma omp parallel default(shared)
+ ;
+ // CHECK-NOTES: :[[@LINE-2]]:9: warning: OpenMP directive 'parallel' is allowed to contain the 'default' clause, and 'default' clause exists with 'shared' kind. Consider using 'default(none)' clause instead.
+ // CHECK-NOTES: :[[@LINE-3]]:22: note: Existing 'default' clause is specified here.
+}
+
+//----------------------------------------------------------------------------//
+// Two directive cases.
+//----------------------------------------------------------------------------//
+
+// 'parallel' directive can have 'default' clause, but said clause is not
+// specified, diagnosed.
+void t4(const int a) {
+#pragma omp parallel for
+ for (int b = 0; b < a; b++)
+ ;
+ // CHECK-NOTES: :[[@LINE-3]]:9: warning: OpenMP directive 'parallel for' is allowed to contain the 'default' clause, but no 'default' clause is specified. Consider specifying 'default(none)' clause.
+}
+
+// 'parallel' directive can have 'default' clause, and said clause is specified,
+// with 'none' kind, all good.
+void t5(const int a) {
+#pragma omp parallel for default(none)
+ for (int b = 0; b < a; b++)
+ ;
+}
+
+// 'parallel' directive can have 'default' clause, and said clause is specified,
+// but with 'shared' kind, which is not 'none', diagnose.
+void t6(const int a) {
+#pragma omp parallel for default(shared)
+ for (int b = 0; b < a; b++)
+ ;
+ // CHECK-NOTES: :[[@LINE-3]]:9: warning: OpenMP directive 'parallel for' is allowed to contain the 'default' clause, and 'default' clause exists with 'shared' kind. Consider using 'default(none)' clause instead.
+ // CHECK-NOTES: :[[@LINE-4]]:26: note: Existing 'default' clause is specified here.
+}
Index: docs/clang-tidy/checks/openmp-use-default-none.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/openmp-use-default-none.rst
@@ -0,0 +1,70 @@
+.. title:: clang-tidy - openmp-use-default-none
+
+openmp-use-default-none
+=======================
+
+Finds OpenMP directives that are allowed to contain ``default`` clause,
+but either don't specify it, or the clause is specified but with the kind
+other than ``none``, and suggests to use ``default(none)`` clause.
+
+Using ``default(none)`` clause changes the default variable visibility from
+being implicitly determined, and thus forces developer to be explicit about the
+desired data scoping for each variable.
+
+Example
+-------
+
+.. code-block:: c++
+
+
+ // 'for' directive can not have 'default' clause, no diagnostics.
+ void t0(const int a) {
+ #pragma omp for
+ for (int b = 0; b < a; b++)
+ ;
+ }
+
+ // 'parallel' directive can have 'default' clause, but said clause is not specified, diagnosed.
+ void t1() {
+ #pragma omp parallel // <- warning: OpenMP directive 'parallel' is allowed to contain the 'default' clause, but no 'default' clause is specified. Consider specifying 'default(none)' clause.
+ ;
+ }
+
+ // 'parallel' directive can have 'default' clause, and said clause is specified, with 'none' kind, all good.
+ void t2() {
+ #pragma omp parallel default(none)
+ ;
+ }
+
+ // 'parallel' directive can have 'default' clause, and said clause is specified but with 'shared' kind, which is not 'none', diagnose.
+ void t3() {
+ #pragma omp parallel default(shared) // <- warning: OpenMP directive 'parallel' is allowed to contain the 'default' clause, and 'default' clause exists with 'shared' kind. Consider using 'default(none)' clause instead.
+ // ^ note: Existing 'default' clause is specified here.
+ ;
+ }
+
+ //----------------------------------------------------------------------------//
+ // Combined directives are handled correctly.
+ //----------------------------------------------------------------------------//
+
+ // 'parallel' directive can have 'default' clause, but said clause is not specified, diagnosed.
+ void t4(const int a) {
+ #pragma omp parallel for
+ for (int b = 0; b < a; b++) // <- warning: OpenMP directive 'parallel for' is allowed to contain the 'default' clause, but no 'default' clause is specified. Consider specifying 'default(none)' clause.
+ ;
+ }
+
+ // 'parallel' directive can have 'default' clause, and said clause is specified with 'none' kind, all good.
+ void t5(const int a) {
+ #pragma omp parallel for default(none)
+ for (int b = 0; b < a; b++)
+ ;
+ }
+
+ // 'parallel' directive can have 'default' clause, and said clause is specified, but with 'shared' kind, which is not 'none', diagnose.
+ void t6(const int a) {
+ #pragma omp parallel for default(shared) // <- warning: OpenMP directive 'parallel for' is allowed to contain the 'default' clause, and 'default' clause exists with 'shared' kind. Consider using 'default(none)' clause instead.
+ // ^ note: Existing 'default' clause is specified here.
+ for (int b = 0; b < a; b++)
+ ;
+ }
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -221,6 +221,7 @@
objc-avoid-spinlock
objc-forbidden-subclassing
objc-property-declaration
+ openmp-use-default-none
performance-faster-string-find
performance-for-range-copy
performance-implicit-conversion-in-loop
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -73,6 +73,16 @@
Checks for casts of ``absl::Duration`` conversion functions, and recommends
the right conversion function instead.
+- New OpenMP module.
+
+ For checks specific to `OpenMP <https://www.openmp.org/>`_ API.
+
+- New :doc:`openmp-use-default-none
+ <clang-tidy/checks/openmp-use-default-none>` check.
+
+ Diagnoses directives that are allowed to contain 'default(none)' clause,
+ but don't contain it.
+
Improvements to include-fixer
-----------------------------
Index: clang-tidy/tool/CMakeLists.txt
===================================================================
--- clang-tidy/tool/CMakeLists.txt
+++ clang-tidy/tool/CMakeLists.txt
@@ -30,6 +30,7 @@
clangTidyMiscModule
clangTidyModernizeModule
clangTidyObjCModule
+ clangTidyOpenMPModule
clangTidyPerformanceModule
clangTidyPortabilityModule
clangTidyReadabilityModule
Index: clang-tidy/plugin/CMakeLists.txt
===================================================================
--- clang-tidy/plugin/CMakeLists.txt
+++ clang-tidy/plugin/CMakeLists.txt
@@ -21,6 +21,7 @@
clangTidyMiscModule
clangTidyModernizeModule
clangTidyObjCModule
+ clangTidyOpenMPModule
clangTidyPerformanceModule
clangTidyPortabilityModule
clangTidyReadabilityModule
Index: clang-tidy/openmp/UseDefaultNoneCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/openmp/UseDefaultNoneCheck.h
@@ -0,0 +1,36 @@
+//===--- UseDefaultNoneCheck.h - clang-tidy ---------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OPENMP_USEDEFAULTNONECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OPENMP_USEDEFAULTNONECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace openmp {
+
+/// Finds OpenMP directives that are allowed to contain ``default`` clause,
+/// but either don``t specify it, or the clause is specified but with the kind
+/// other than ``none``, and suggests to use ``default(none)`` clause.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/openmp-use-default-none.html
+class UseDefaultNoneCheck : public ClangTidyCheck {
+public:
+ UseDefaultNoneCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace openmp
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OPENMP_USEDEFAULTNONECHECK_H
Index: clang-tidy/openmp/UseDefaultNoneCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/openmp/UseDefaultNoneCheck.cpp
@@ -0,0 +1,155 @@
+//===--- UseDefaultNoneCheck.cpp - clang-tidy -----------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "UseDefaultNoneCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/OpenMPClause.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/StmtOpenMP.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+
+namespace {
+
+/// Matches any '#pragma omp' executable directive.
+///
+/// Example:
+///
+/// \code
+/// #pragma omp parallel
+/// #pragma omp for <...>
+/// \endcode
+const ast_matchers::internal::VariadicDynCastAllOfMatcher<
+ Stmt, OMPExecutableDirective>
+ ompExecutableDirective;
+
+/// Matches if the OpenMP directive is allowed to contain the specified OpenMP
+/// clause kind.
+///
+/// Example:
+///
+/// Given, `ompExecutableDirective(isAllowedToContainClause(OMPC_default))`:
+///
+/// \code
+/// #pragma omp parallel // <- match
+/// #pragma omp parallel for // <- match
+/// #pragma omp for // <- no match
+/// \endcode
+///
+/// FIXME: would be better to pass the actual class name (e.g. OMPDefaultClause)
+/// instead of the actual OpenMPClauseKind.
+AST_MATCHER_P(OMPExecutableDirective, isAllowedToContainClause,
+ OpenMPClauseKind, CKind) {
+ return isAllowedClauseForDirective(Node.getDirectiveKind(), CKind);
+}
+
+/// Matches first matching OpenMP clause that matches InnerMatcher.
+///
+/// Example:
+///
+/// Given 'ompExecutableDirective(hasClause(anything()))':
+///
+/// \code
+/// #pragma omp parallel // <- no clauses, no match
+/// #pragma omp parallel default(none) // <- has clauses, match
+/// \endcode
+AST_MATCHER_P(OMPExecutableDirective, hasClause,
+ ast_matchers::internal::Matcher<OMPClause>, InnerMatcher) {
+ ArrayRef<OMPClause *> Clauses = Node.clauses();
+ return matchesFirstInPointerRange(InnerMatcher, Clauses.begin(),
+ Clauses.end(), Finder, Builder);
+}
+
+/// Matches OpenMP 'default(...)' clause.
+///
+/// Example:
+///
+/// \code
+/// #pragma omp <...> default(none) // <- will match 'default(none)'
+/// #pragma omp <...> default(shared) // <- will match 'default(shared)'
+/// #pragma omp <...> // <- no match
+/// \endcode
+const ast_matchers::internal::VariadicDynCastAllOfMatcher<OMPClause,
+ OMPDefaultClause>
+ ompDefaultClause;
+
+/// Matches the specific default kind in the OpenMP 'default(...)' clause.
+///
+/// Example:
+///
+/// Given, `ompDefaultClause(hasKind(OMPC_DEFAULT_none))`:
+///
+/// \code
+/// #pragma omp parallel default(none) // <- match
+/// #pragma omp parallel default(shared) // <- no match
+/// \endcode
+///
+/// Given, `ompDefaultClause(hasKind(OMPC_DEFAULT_shared))`:
+///
+/// \code
+/// #pragma omp parallel default(none) // <- no match
+/// #pragma omp parallel default(shared) // <- match
+/// \endcode
+AST_MATCHER_P(OMPDefaultClause, hasKind, OpenMPDefaultClauseKind, Kind) {
+ return Node.getDefaultKind() == Kind;
+}
+
+} // namespace
+
+namespace tidy {
+namespace openmp {
+
+void UseDefaultNoneCheck::registerMatchers(MatchFinder *Finder) {
+ // If OpenMP is not enabled, don't register the check, it won't find anything.
+ if (!getLangOpts().OpenMP)
+ return;
+
+ Finder->addMatcher(
+ ompExecutableDirective(
+ allOf(isAllowedToContainClause(OMPC_default),
+ anyOf(unless(hasClause(ompDefaultClause())),
+ hasClause(
+ ompDefaultClause(unless(hasKind(OMPC_DEFAULT_none)))
+ .bind("clause")))))
+ .bind("directive"),
+ this);
+}
+
+void UseDefaultNoneCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *Directive =
+ Result.Nodes.getNodeAs<OMPExecutableDirective>("directive");
+ assert(Directive != nullptr);
+
+ if (const auto *Clause = Result.Nodes.getNodeAs<OMPDefaultClause>("clause")) {
+ diag(Directive->getBeginLoc(),
+ "OpenMP directive '%0' is allowed to contain the 'default' clause, "
+ "and 'default' clause exists with '%1' kind. Consider using "
+ "'default(none)' clause instead.")
+ << getOpenMPDirectiveName(Directive->getDirectiveKind())
+ << getOpenMPSimpleClauseTypeName(Clause->getClauseKind(),
+ Clause->getDefaultKind());
+ diag(Clause->getBeginLoc(), "Existing 'default' clause is specified here.",
+ DiagnosticIDs::Note);
+ return;
+ }
+
+ diag(Directive->getBeginLoc(),
+ "OpenMP directive '%0' is allowed to contain the 'default' clause, but "
+ "no 'default' clause is specified. Consider specifying 'default(none)' "
+ "clause.")
+ << getOpenMPDirectiveName(Directive->getDirectiveKind());
+}
+
+} // namespace openmp
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/openmp/OpenMPTidyModule.cpp
===================================================================
--- /dev/null
+++ clang-tidy/openmp/OpenMPTidyModule.cpp
@@ -0,0 +1,38 @@
+//===--- OpenMPTidyModule.cpp - clang-tidy--------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+#include "UseDefaultNoneCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace openmp {
+
+/// This module is for OpenMP-specific checks.
+class OpenMPModule : public ClangTidyModule {
+public:
+ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<UseDefaultNoneCheck>(
+ "openmp-use-default-none");
+ }
+};
+
+// Register the OpenMPTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<OpenMPModule>
+ X("openmp-module", "Adds OpenMP-specific checks.");
+
+} // namespace openmp
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the OpenMPModule.
+volatile int OpenMPModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/openmp/CMakeLists.txt
===================================================================
--- /dev/null
+++ clang-tidy/openmp/CMakeLists.txt
@@ -0,0 +1,12 @@
+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyOpenMPModule
+ OpenMPTidyModule.cpp
+ UseDefaultNoneCheck.cpp
+
+ LINK_LIBS
+ clangAST
+ clangASTMatchers
+ clangBasic
+ clangTidy
+ )
Index: clang-tidy/ClangTidyForceLinker.h
===================================================================
--- clang-tidy/ClangTidyForceLinker.h
+++ clang-tidy/ClangTidyForceLinker.h
@@ -77,6 +77,11 @@
MPIModuleAnchorSource;
#endif
+// This anchor is used to force the linker to link the OpenMPModule.
+extern volatile int OpenMPModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED OpenMPModuleAnchorDestination =
+ OpenMPModuleAnchorSource;
+
// This anchor is used to force the linker to link the PerformanceModule.
extern volatile int PerformanceModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED PerformanceModuleAnchorDestination =
Index: clang-tidy/CMakeLists.txt
===================================================================
--- clang-tidy/CMakeLists.txt
+++ clang-tidy/CMakeLists.txt
@@ -49,6 +49,7 @@
add_subdirectory(mpi)
endif()
add_subdirectory(objc)
+add_subdirectory(openmp)
add_subdirectory(performance)
add_subdirectory(plugin)
add_subdirectory(portability)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
