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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.cpp:49-50
+///
+/// FIXME: would be better to pass the actual class name (e.g. OMPDefaultClause)
+///        instead of the actual OpenMPClauseKind.
+AST_MATCHER_P(OMPExecutableDirective, isAllowedToContainClause,
----------------
Solved, this is so ugly xD


================
Comment at: clang-tidy/openmp/UseDefaultNoneCheck.cpp:97
+///
+/// Given, `ompDefaultClause(hasKind(OMPC_DEFAULT_shared))`:
+///
----------------
lebedev.ri wrote:
> 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!
Not quite sure re `Kind` suffix, but i guess this is better.


================
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)
----------------
ABataev wrote:
> JonasToth wrote:
> > 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.
> If OpenMP is disabled, the pragmas are just completely ignored.
Reworded a little, hopefully better.


================
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.
----------------
JonasToth wrote:
> double back-tick in dont, please use the normal straight tick instead
Hah, that is mass-replace for you.


================
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.
+
----------------
lebedev.ri wrote:
> JonasToth wrote:
> > lebedev.ri wrote:
> > > JonasToth wrote:
> > > > 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?
> > > I'm not quite sure how to formulate the reasoning to be honest.
> > > Let me try to show some reasonably-complete example:
> > > https://godbolt.org/z/mMQhbr
> > > 
> > > We have 4 compilers: clang trunk + openmp 3.1, clang trunk + openmp 4, gcc 8, gcc trunk
> > > 
> > > We have 5 code samples, all operating with a `const` variable: (the same code, just different omp clauses)
> > > * If no `default` clause is specified, every compiler is fine with the code.
> > > * If `default(shared)` clause is specified, every compiler is still fine with the code.
> > >   On this example, no clause and `default(shared)` clause are equivalent.
> > >   I can't/won't claim whether or not that is always the case, as i'm mostly arguing re `default(none)`.
> > > * If `default(none) shared(num)` is specified, all is also fine.
> > >   That is equivalent to just the `default(none)` for OpenMP 3.1
> > > * If `default(none) firstprivate(num)` is specified, all still fine fine.
> > > * If only ``default(none)` is specified, things start to get wonky.
> > >   The general idea is that before OpenMP 4.0 such `const` variables were auto-determined as `shared`,
> > >   and now they won't. So one will need to add either `shared(num)` or `firstprivate(num)`.
> > >   Except the older gcc will diagnose `shared(num)` :)
> > > 
> > > Roughly the same is true when the variable is not `const`: https://godbolt.org/z/wuouI_
> > > 
> > > Thus, `default(none)` forced one to be explicit about what shall be done with the variable,
> > > should it be `shared`, or `firstprivate`.
> > > 
> > > 
> > > ^ Not whether this rambling makes sense?
> > Yes, so its about being explicit if state is shared or not. Given that its hard to reason about parallel programs being explicit helps reading the code. I think that could be condensed in a short motivation section.
> > Yes, so its about being explicit if state is shared or not. Given that its hard to reason about parallel programs being explicit helps reading the code. I think that could be condensed in a short motivation section.
> 
> Yep. Will try to reword.
> 
> > 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?
> 
> E.g. clang messages say `variable 'num' must have explicitly specified data sharing attributes`
> So "data sharing" i suppose.
Reworded, hopefully better?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57113