[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

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:
> > 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`.


================
Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
----------------
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112