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