lebedev.ri marked 5 inline comments as done.
lebedev.ri 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.
----------------
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.
================
Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+ EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
----------------
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"
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
