JonasToth added inline comments.
================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.cpp:113
+void UseDefaultNoneCheck::registerMatchers(MatchFinder *Finder) {
+ // If OpenMP is not enabled, don't register the check, it won't find
anything.
+ if (!getLangOpts().OpenMP)
----------------
Is that the case? Will the pragmas be ignored if non-omp?
You could reorder that sentence `Don't register the check if OpenMP is not
enabled as the directives are not considered in the AST.` or so.
================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.cpp:131
+ Result.Nodes.getNodeAs<OMPExecutableDirective>("directive");
+ assert(Directive != nullptr);
+
----------------
lebedev.ri wrote:
> JonasToth wrote:
> > assert without message, but probably redundant anyway, because the matcher
> > can only fire if `directive` matched.
> Hm, i can drop it, but in previous reviews i have seen this assert being
> requested to be *added*.
You can leave it, but please add an message to the assertion.
================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.h:18
+
+/// 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
----------------
contain _a_ ``default`` clause
================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.h:19
+/// 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.
----------------
double back-tick in dont, please use the normal straight tick instead
================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.h:20
+/// 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.
+///
----------------
to use _the_
================
Comment at: docs/clang-tidy/checks/openmp-use-default-none.rst:6
+
+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
----------------
not sure, but i think all the commas in this sentence can be dropped. The comma
in front of `or` for sure.
================
Comment at: docs/clang-tidy/checks/openmp-use-default-none.rst:11
+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.
----------------
forces _developers_
================
Comment at: docs/clang-tidy/checks/openmp-use-default-none.rst:12
+being implicitly determined, and thus forces developer to be explicit about the
+desired data scoping for each variable.
+
----------------
JonasToth wrote:
> If I understand correctly the issue is more about implicitly shared variables
> that lead to data-races and bad access patterns, is that correct?
> If so it might clarify the reason for this check, if added directly in the
> first motivational sentence.
is it scoping or rather sharing? The scope is C++-terrain or at least used in
C++-Speak. Maybe there is a unambiguous word instead?
================
Comment at: docs/clang-tidy/checks/openmp-use-default-none.rst:28
+ 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.
+ ;
----------------
Very long line. You can move the warning to the next line and wrap it to match
the 80 columns limit.
================
Comment at: test/clang-tidy/openmp-use-default-none.cpp:53
+// 'parallel' directive can have 'default' clause, and said clause is
specified,
+// with 'none' kind, all good.
+void t5(const int a) {
----------------
Please add basic test-cases for the other constructs that are allowed to have
the `default` specified.
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
