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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 4 inline comments as done.
lebedev.ri added inline comments.


================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.cpp:51
+///        instead of the actual OpenMPClauseKind.
+AST_MATCHER_P(OMPExecutableDirective, isAllowedToContainClause,
+              OpenMPClauseKind, CKind) {
----------------
JonasToth wrote:
> Why is this required? If it is not allowed, is the shouldn't that be a compilation error already (always assuming openmp is activated).
It's tricky logic.
There are three possible scenarios here:
* `default(none)` is specified, all good.
* `default(shared)` is specified, `shared`!=`none`, diagnose.
* No `default` clause specified.
  `Only a single default clause may be specified on a parallel, task, taskloop or teams directive.` ([[ https://www.openmp.org/wp-content/uploads/OpenMP-API-Specification-5.0.pdf | `2.19.4.1 default Clause` ]], last line of paragraph).
  Now, there are two possible cases:
  * We are in a directive (e.g. `parallel`) that **is** allowed to have the `default` clause, but does not have it. Naturally, do diagnose this.
  * We are in a directive (e.g. `for`) that is **not** allowed to have the `default` clause. Naturally, don't diagnose this.

So this is correct.


================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.cpp:97
+///
+/// Given, `ompDefaultClause(hasKind(OMPC_DEFAULT_shared))`:
+///
----------------
JonasToth wrote:
> you could provide helper-matchers similiar to `isImplicit()`, `isInline()` to allow `ompDefaultClause(isDefaultShared())`.
Could work, will take a look, thanks for a hint!


================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.cpp:131
+      Result.Nodes.getNodeAs<OMPExecutableDirective>("directive");
+  assert(Directive != nullptr);
+
----------------
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*.


================
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 "
----------------
JonasToth wrote:
> 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.
Good point about "allowed", i'll see how this can be trimmed..


================
Comment at: test/clang-tidy/openmp-use-default-none.cpp:26
+void t2() {
+#pragma omp parallel default(none)
+  ;
----------------
JonasToth wrote:
> AFAIK `default(private)` should exist as well, please add tests for the other kinds, too.
In Fortran - yes, but not in C / C++:
https://www.openmp.org/wp-content/uploads/OpenMP-API-Specification-5.0.pdf
Page 282 (302 in pdf):
```
2.19.4.1 default Clause
...
C / C++
The syntax of the default clause is as follows:
default(shared | none)

```



================
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.
+}
----------------
JonasToth wrote:
> why rewrite the `default(shared)`? I don't exactly understand the reason for not accepting `default(shared)`.
This check enforces `default(none)`. If the default of `shared` is specified,
that is still not `none`, therefore it is incorrect and should be diagnosed.

Why not `shared` but `none`? I tried to cover that in the doc,
basically it helps prevent issues by forcing one to be explicit about the "visibilities"
of the variables.

I suppose it the default can be configurable,
i'm just not aware of any reason why one would specify `shared`.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57113