gribozavr added inline comments.
================
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.
----------------
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.
================
Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+ EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
----------------
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.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57112/new/
https://reviews.llvm.org/D57112
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
