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