Page MenuHomePhabricator

[ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling
ClosedPublic

Authored by lebedev.ri on Jan 23 2019, 11:56 AM.

Details

Summary

OMPClause is the base class, it is not descendant from any other class,
therefore for it to work with e.g. VariadicDynCastAllOfMatcher<>, it needs to be handled here.

Now, i'm not quite sure what else should be here,
i have simply followed the preexisting, prevalent pattern in these source files.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
lebedev.ri added inline comments.Jan 26 2019, 1:53 AM
lib/AST/ASTTypeTraits.cpp
114 ↗(On Diff #183138)

D57280, will rebase this.

lebedev.ri marked an inline comment as done.

Rebased ontop of D57280, use const auto*.

Store OpenMP clause spelling in the ASTNodeKind::KindInfo ASTNodeKind::AllKindInfo[], not the stringified clang AST class name.

lebedev.ri added inline comments.Jan 27 2019, 5:09 AM
lib/AST/ASTTypeTraits.cpp
40–43 ↗(On Diff #183692)

Humm, though i guess this will need to be changed back for when the matchers evolve
from the check into the ASTMatchers, for better clang-query expirience.

Would it be ok to add the opposite direction functionality
as compared to static ASTNodeKind getFromNode(const OMPClause &C);?
I.e. roughly OMPClauseKind getOMPClauseKindFromASTNodeKind() const; ?

lebedev.ri marked an inline comment as done.
  • Rebased
  • Go back to storing the actual AST class type as string in ASTNodeKind::KindInfo ASTNodeKind::AllKindInfo[].
  • Add OpenMPClauseKind ASTNodeKind::getOMPClauseKind() to address the regression of D57113 due to the previous ^ change. This is better anyway, since it avoids string comparisons, and is a simple switch over an enum.

Ping. I'm not quite sure who is the best suited to review this..

Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 1:22 AM

Ping @hokein / @alexfh (as per git blame).
Not sure who is best suited to review this.

Ping @hokein / @alexfh (as per git blame).
Not sure who is best suited to review this.

I only made a couple of random fixes to these files, so I don't feel particularly competent to review them. Though overall the changes in this patch seem reasonable to me. No concerns from me ;)

Maybe ping @klimek then? :)

Ping @hokein / @alexfh (as per git blame).
Not sure who is best suited to review this.

I only made a couple of random fixes to these files, so I don't feel particularly competent to review them. Though overall the changes in this patch seem reasonable to me. No concerns from me ;)

Makes sense, thanks for looking!

ping @klimek @hokein thanks.
The fun of of the code that is so well separated that no one
touches it for years and thus can't review patches for it :)

The test coverage i have will be in the clang-tidy check.

Please add unit tests to clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp. You can pull matchers from D57113 into the shared matchers library (instead of keeping them private to the check), and add unit tests.

include/clang/AST/ASTTypeTraits.h
83 ↗(On Diff #183902)

"asOMPClauseKind()"?

lib/AST/ASTTypeTraits.cpp
114 ↗(On Diff #183138)

Well, I think it would be good to filter out OMPC_flush somehow because there is no such clause actually, it is a pseudo clause for better handling of the flush directive.

Sorry to be late for this discussion, but I don't think this conclusion follows. ASTMatchers are supposed to match the AST as it is. Even if OMPC_flush is synthetic, it exists in the AST, and users might want to match it. I think users would find anything else (trying to filter out AST nodes that are not in the source code) to be surprising. For example, there's a matcher materializeTemporaryExpr even though this AST node is a Clang invention and is not a part of the C++ spec.

Matching only constructs that appear in the source code is not feasible with ASTMatchers, because they are based on Clang's AST that exposes tons of semantic information, and its design is dictated by the structure of the semantic information. See "RFC: Tree-based refactorings with Clang" in cfe-dev for a library that will focus on representing source code as faithfully as possible.

Not to even mention that this code is in ASTTypeTraits, a general library for handling AST nodes, not specifically for AST Matchers...

41 ↗(On Diff #183902)

Why "DERIVED"? The definition of OPENMP_CLAUSE calls the second argument "Class" which I think is more appropriate.

70 ↗(On Diff #183902)

Why "DERIVED"? The definition of OPENMP_CLAUSE calls the second argument "Class" which I think is more appropriate.

lebedev.ri marked 6 inline comments as not done.Mar 4 2019, 7:31 AM
lebedev.ri added inline comments.
lib/AST/ASTTypeTraits.cpp
114 ↗(On Diff #183138)

So, uh, i should have checked beforehand.
https://godbolt.org/z/aanQ8U
D57280 is unjustified (and thus incorrect) and needs to be reverted.

lebedev.ri marked 3 inline comments as done.
lebedev.ri retitled this revision from [ASTTypeTraits] OMPClause handling to [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling.
lebedev.ri edited the summary of this revision. (Show Details)

Split off matchers from from D57113, added tests.

aaron.ballman added inline comments.Mar 16 2019, 11:49 AM
include/clang/AST/ASTTypeTraits.h
81 ↗(On Diff #190965)

These markings are a bit strange, can you explain them to me?

include/clang/ASTMatchers/ASTMatchers.h
6389 ↗(On Diff #190965)

Given OpenMP -> Given an OpenMP

lib/ASTMatchers/Dynamic/Registry.cpp
509–512 ↗(On Diff #190965)

Please keep these alphabetically sorted.

lebedev.ri marked an inline comment as done.Mar 16 2019, 11:56 AM
lebedev.ri added inline comments.
include/clang/AST/ASTTypeTraits.h
81 ↗(On Diff #190965)

It is weird, but i think this is the right solution.
See isAllowedToContainClause() narrower.
This asOMPClauseKind() allows to pass a whole matcher, and then distill the inner output node type.
I.e. now i can spell ompExecutableDirective(isAllowedToContainClause(ompDefaultClause()))
(and the ompDefaultClause() won't actually be used for matching!), instead of doing something like e.g.
ompExecutableDirective(isAllowedToContainClause(OMPC_default))
which looks horrible, and will likely not work well with clang-query?

aaron.ballman added inline comments.Mar 17 2019, 7:18 AM
include/clang/AST/ASTTypeTraits.h
81 ↗(On Diff #190965)

I think we may be talking about different things. I only meant the \{ and \} comment markers. :-D I think the design is reasonable enough, but I'm not an OpenMP expert. Truth be told, I was mostly surprised that these pragmas will have corresponding AST matchers. I thought they were preprocessor directives and thus would be handled at that level, rather than the AST level. My only concern about the design of this is a pretty minor one: what happens if someone is using preprocessor callbacks and AST matchers at the same time -- will they get duplicate results for OpenMP directives? I suspect they will, but that doesn't mean we shouldn't AST match OpenMP clauses (they are in the AST, after all).

lebedev.ri marked 3 inline comments as done.Mar 17 2019, 7:25 AM
lebedev.ri added inline comments.
include/clang/AST/ASTTypeTraits.h
81 ↗(On Diff #190965)

Oh duh xD
I just copied them from around the getFromNode() up there ^.
I believe they signify that these functions should be displayed as a group in doxygen.

I don't know what will happen when using preprocessor callbacks and AST matchers at the same time,
but i think that would be the issue of the user in question.

aaron.ballman added inline comments.Mar 17 2019, 7:33 AM
include/clang/AST/ASTTypeTraits.h
81 ↗(On Diff #190965)

I just copied them from around the getFromNode() up there ^.
I believe they signify that these functions should be displayed as a group in doxygen.

I thought so as well, but there's no grouping here. I'd probably drop them until it's needed.

I don't know what will happen when using preprocessor callbacks and AST matchers at the same time, but i think that would be the issue of the user in question.

I agree.

gribozavr added inline comments.Mar 18 2019, 3:08 AM
include/clang/ASTMatchers/ASTMatchers.h
6390 ↗(On Diff #190965)

Other comments usually don't explain the interactions with inner matchers, unless the semantics are unusual.

"Matches any clause in an OpenMP directive."

6400 ↗(On Diff #190965)

Looking at other similar matches, they usually follow the naming pattern hasAny~, WDYT?

(hasAnyDeclaration, hasAnyConstructorInitializer etc.)

6411 ↗(On Diff #190965)

Please follow the existing comment style. Either:

Given

\code
<code snippet>
\endcode

<matcher expression> matches "<code snippet>".

or

Example: <matcher expression> matches "<code snippet>" in 

\code
<code snippet>
\endcode

For example:

Given

 \code
   #pragma omp parallel default(none)
   #pragma omp parallel default(shared)
   #pragma omp parallel
 \endcode

``ompDefaultClause()`` matches ``default(none)` and ``default(shared)``.

Similarly for other comments in this patch.

6433 ↗(On Diff #190965)

Also add isSharedKind? It is weird that we have one but not the other.

6449 ↗(On Diff #190965)

"while this matcher"

6456 ↗(On Diff #190965)

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.)

unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
2283 ↗(On Diff #190965)

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.

lebedev.ri marked 4 inline comments as done.Mar 18 2019, 3:19 AM
lebedev.ri added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
6456 ↗(On Diff #190965)

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?

unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
2283 ↗(On Diff #190965)

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.

gribozavr added inline comments.Mar 18 2019, 3:32 AM
include/clang/ASTMatchers/ASTMatchers.h
6456 ↗(On Diff #190965)

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.

unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
2283 ↗(On Diff #190965)

Other tests in this file use string concatenation, see TEST(DeclarationMatcher, MatchHasRecursiveAllOf) for example.

lebedev.ri marked 5 inline comments as done.Mar 18 2019, 3:45 AM
lebedev.ri added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
6456 ↗(On Diff #190965)

True. Also, but there's dosens of Stmt types, and there is no overload that takes StmtClass enum.

unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
2283 ↗(On Diff #190965)

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"

gribozavr added inline comments.Mar 18 2019, 4:01 AM
include/clang/ASTMatchers/ASTMatchers.h
6456 ↗(On Diff #190965)

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.

unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
2283 ↗(On Diff #190965)

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.

lebedev.ri marked 3 inline comments as done.Mar 18 2019, 4:08 AM
lebedev.ri added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
6456 ↗(On Diff #190965)

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.

gribozavr added inline comments.Mar 18 2019, 4:15 AM
include/clang/ASTMatchers/ASTMatchers.h
6456 ↗(On Diff #190965)

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.

aaron.ballman added inline comments.Mar 18 2019, 5:37 AM
include/clang/ASTMatchers/ASTMatchers.h
6400 ↗(On Diff #190965)

Yeah, I think this would be a good change to make. Good catch!

6433 ↗(On Diff #190965)

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.

6456 ↗(On Diff #190965)

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.

unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
2283 ↗(On Diff #190965)

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.

lebedev.ri marked 18 inline comments as done.

Rebased, addressed all(?) nits.

include/clang/ASTMatchers/ASTMatchers.h
6456 ↗(On Diff #190965)

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

unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
2283 ↗(On Diff #190965)

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.

gribozavr accepted this revision.Mar 21 2019, 7:13 AM
This revision is now accepted and ready to land.Mar 21 2019, 7:13 AM

@gribozavr thank you for the review!
@aaron.ballman any further comments?

aaron.ballman added inline comments.Mar 21 2019, 7:16 AM
include/clang/ASTMatchers/ASTMatchers.h
6456 ↗(On Diff #190965)

clang-query requires enumerations to be quoted string literals. If you switch to that in your test, does it work for you? I was spotting some odd behavior with a different matcher (the attribute one, which documents the quoting requirement).

6473–6475 ↗(On Diff #191678)

See the matcher for hasAttr() -- we should use similar exposition here.

lebedev.ri marked an inline comment as done.

Address @aaron.ballman comment nit.

aaron.ballman accepted this revision.Mar 21 2019, 8:04 AM

LGTM aside from a small docs nit (be sure to regenerate the docs after you fix it).

include/clang/ASTMatchers/ASTMatchers.h
6402 ↗(On Diff #191685)

hasAnyClause()

lebedev.ri marked 2 inline comments as done.

Comment nit.

include/clang/ASTMatchers/ASTMatchers.h
6402 ↗(On Diff #191685)

whoops, thanks.

LGTM

Great, thank you for the review!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2019, 8:32 AM