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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
