aaron.ballman added inline comments.
================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:6400
+/// \endcode
+AST_MATCHER_P(OMPExecutableDirective, hasClause, internal::Matcher<OMPClause>,
+ InnerMatcher) {
----------------
gribozavr wrote:
> Looking at other similar matches, they usually follow the naming pattern
> `hasAny~`, WDYT?
>
> (`hasAnyDeclaration`, `hasAnyConstructorInitializer` etc.)
Yeah, I think this would be a good change to make. Good catch!
================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:6433
+ return Node.getDefaultKind() == OMPC_DEFAULT_none;
+}
+
----------------
gribozavr wrote:
> Also add `isSharedKind`? It is weird that we have one but not the other.
Given that there's only two options, it's not strictly required to have them
both. But I don't see a reason not to add it, either.
================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+ // Note that we have recieved a *matcher* for the clause, not the
+ // OpenMPClauseKind. We now need to extract the 'return' type of said
matcher,
+ // and convert it to the OpenMPClauseKind, so we can finally use that.
----------------
gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > lebedev.ri wrote:
> > > > gribozavr wrote:
> > > > > lebedev.ri wrote:
> > > > > > gribozavr wrote:
> > > > > > > Why not make `isAllowedToContainClause` take an
> > > > > > > `OpenMPClauseKind` enum value?
> > > > > > >
> > > > > > > I don't see right now advantages for taking a matcher. (For
> > > > > > > example, it can't be a more complex matcher with inner matchers,
> > > > > > > it can't be a disjunction of matchers etc.)
> > > > > > I don't feel like it, it's uglier.
> > > > > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > > >
> > > > > There are dozens of clauses in `OpenMPClauseKind`. We would have to
> > > > > replicate them all as matchers to provide a useful API.
> > > > >
> > > > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > >
> > > > > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> > > > True. Also, but there's dosens of Stmt types, and there is no overload
> > > > that takes `StmtClass` enum.
> > > For Stmts, we do have dozens of individual matchers for them.
> > >
> > > The point of your work is to add ASTMatchers for OpenMP, right? However,
> > > if there are no matchers for a reasonable amount of AST surface, it is as
> > > good as if the matchers are not there, because prospective users won't be
> > > able to use them.
> > >
> > > I don't particularly care how exactly this is achieved, through
> > > individual matchers or through a matcher that takes an enum. However, I
> > > want to make sure that if you're going through all this trouble to add
> > > matchers, the resulting API should cover a good amount of AST.
> > >
> > > The reason why I suggested to pass the enum to the matcher is simply
> > > because it is less code duplication, less work, and more reliable code
> > > (since there will be only one matcher to review, test, and maintain,
> > > instead of combinations of matchers).
> > >
> > > Another reason to not use an inner matcher here is the peculiar semantics
> > > of this function -- it does not evaluate the matcher, and it does not
> > > accept a matcher expression of any shape.
> > > The point of your work is to add ASTMatchers for OpenMP, right?
> >
> > Absolutely not.
> > D57113 + D59466 is the one and only point, to address the bugs i have
> > personally encountered.
> > The whole reason why i have started off with NOT adding these matchers to
> > the `ASTMatchers.h`,
> > but keeping them at least initially internal to the checks was to avoid all
> > this bikeshedding.
> However, I do care about the AST matchers being usable by other clients.
>
> I also care about the API following existing patterns:
>
> > Another reason to not use an inner matcher here is the peculiar semantics
> > of this function -- it does not evaluate the matcher, and it does not
> > accept a matcher expression of any shape.
>
>
>> Also, how will passing some random enum work with e.g. clang-query?
> See llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h.
That doesn't mean it works super well, though. String literals more easily
contain silent typos, don't have autocomplete support, etc. I can definitely
sympathize with not wanting to use an enum here.
However, I see that there are 50+ enumerations in this list -- that seems like
too many matchers to want to expose. I think an enum will be the better, more
maintainable option. The current approach won't scale well.
================
Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+ EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
----------------
gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > lebedev.ri wrote:
> > > > gribozavr wrote:
> > > > > I'm not sure if breaking out the source code into the "SourceX"
> > > > > variables improves readability. WDYT about inlining the code into
> > > > > the EXPECT_TRUE code like in other tests in this file?
> > > > >
> > > > > If you want to break it out, I'd suggest to drop "`void x() {`" down
> > > > > to the next line, so that all code lines start at the same column.
> > > > > I'm not sure if breaking out the source code into the "SourceX"
> > > > > variables improves readability
> > > >
> > > > It's not about readability. Inlining will break the build, rC354201.
> > > Other tests in this file use string concatenation, see
> > > `TEST(DeclarationMatcher, MatchHasRecursiveAllOf)` for example.
> > I'm sorry, but i fail to see how that is relevant?
> > I'm using multiline raw string literals, and inlining it will break the
> > build, like i linked.
> > You are pointing at the code that is not using multiline raw string
> > literals.
> > You only suggested inlining, not switching away from multiline raw string
> > literals, i believe?
> >
> > Not using multiline raw string literals looked even worse, because then you
> > need to manually add "\n"
> > You only suggested inlining, not switching away from multiline raw string
> > literals, i believe?
>
> I am now suggesting to switch away from raw string literals.
>
> > Not using multiline raw string literals looked even worse, because then you
> > need to manually add "\n"
>
> I believe that adding "\n" manually is better than having lots of
> similarly-named SourceX variables, which can easily cause copy-paste mistakes
> (define a SourceX variable, use SourceY in the EXPECT_TRUE line).
>
> However, this is a minor point, up to you. I only wanted to explain my
> reasoning why I prefer inline code snippets.
FWIW, I tend to prefer avoiding local variables and just inline the string
literal into the matches() arguments. The local variable adds no value unless
the snippet is very complex.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57112/new/
https://reviews.llvm.org/D57112
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
