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.
----------------
aaron.ballman wrote:
> 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.
Okay, but apparently clang-query will needs to be fixed too:
```
clang-query> match
stmt(ompExecutableDirective(isAllowedToContainClause(OMPC_default)))
1:1: Error parsing argument 1 for matcher stmt.
1:6: Error parsing argument 1 for matcher ompExecutableDirective.
1:29: Error parsing argument 1 for matcher isAllowedToContainClause.
1:58: Error parsing matcher. Found token <_> while looking for '('.
```
https://bugs.llvm.org/show_bug.cgi?id=41176
================
Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+ EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+
----------------
aaron.ballman wrote:
> 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.
Same as with D59214, i have put the first line of the multiline string literal
on the first line.
I can not inline it into the call, because that call is within the macro, and
as i have linked in that issue, that will break build.
I do not believe avoiding multiline string literal (and thus explicitly
spelling every "\n") would be better.
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
