[PATCH] D57113: [clang-tidy] openmp-use-default-none - a new module and a check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.cpp:51
+///        instead of the actual OpenMPClauseKind.
+AST_MATCHER_P(OMPExecutableDirective, isAllowedToContainClause,
+              OpenMPClauseKind, CKind) {
----------------
Why is this required? If it is not allowed, is the shouldn't that be a compilation error already (always assuming openmp is activated).


================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.cpp:97
+///
+/// Given, `ompDefaultClause(hasKind(OMPC_DEFAULT_shared))`:
+///
----------------
you could provide helper-matchers similiar to `isImplicit()`, `isInline()` to allow `ompDefaultClause(isDefaultShared())`.


================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.cpp:131
+      Result.Nodes.getNodeAs<OMPExecutableDirective>("directive");
+  assert(Directive != nullptr);
+
----------------
assert without message, but probably redundant anyway, because the matcher can only fire if `directive` matched.


================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.cpp:135
+    diag(Directive->getBeginLoc(),
+         "OpenMP directive '%0' is allowed to contain the 'default' clause, "
+         "and 'default' clause exists with '%1' kind. Consider using "
----------------
the message is too long, how about `consider specifiying clause 'default(none)' explicitly` or so?
Adding that it would be allowed is redundant, as one expects the tool to know that and consider that correctly and not diagnose otherwise.


================
Comment at: test/clang-tidy/openmp-use-default-none.cpp:26
+void t2() {
+#pragma omp parallel default(none)
+  ;
----------------
AFAIK `default(private)` should exist as well, please add tests for the other kinds, too.


================
Comment at: test/clang-tidy/openmp-use-default-none.cpp:67
+  // 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.
+}
----------------
why rewrite the `default(shared)`? I don't exactly understand the reason for not accepting `default(shared)`.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57113/new/

https://reviews.llvm.org/D57113