[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: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