Page MenuHomePhabricator

[ASTMatchers][OpenMP] OpenMP Structured-block-related matchers
ClosedPublic

Authored by lebedev.ri on Mar 16 2019, 11:16 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Mar 16 2019, 11:16 AM
aaron.ballman added inline comments.Mar 16 2019, 11:51 AM
include/clang/ASTMatchers/ASTMatchers.h
6421 ↗(On Diff #190982)

What happens if this prereq is not met? Does the matcher return false, or does it do something worse (like assert)?

lib/ASTMatchers/Dynamic/Registry.cpp
513–515 ↗(On Diff #190982)

Alphabetical order, please.

lebedev.ri added inline comments.Mar 16 2019, 11:59 AM
include/clang/ASTMatchers/ASTMatchers.h
6421 ↗(On Diff #190982)

From https://reviews.llvm.org/D59214#change-5GOe7CiMwWxl

const Stmt *OMPExecutableDirective::getStructuredBlock() const {
  assert(!isStandaloneDirective() &&
         "Standalone Executable Directives don't have Structured Blocks.");

It really doesn't make sense to return false/nullptr,
it would be like trying to find ForLoop in VarDecl, they are just really distinct types.

lebedev.ri added inline comments.Mar 17 2019, 12:09 AM
include/clang/ASTMatchers/ASTMatchers.h
6421 ↗(On Diff #190982)

Hmm, actually, i guess it would be best to return false here instead then. Will change.

gribozavr added inline comments.Mar 18 2019, 3:33 AM
include/clang/ASTMatchers/ASTMatchers.h
6389 ↗(On Diff #190982)

"Matches standalone OpenMP directives, i.e., directives that can't have a structured block."

6400 ↗(On Diff #190982)

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.

lebedev.ri marked 6 inline comments as done.

Rebased, addressed all(?) nits.

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

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

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

LGTM aside from a NFC change

include/clang/ASTMatchers/ASTMatchers.h
6444–6445 ↗(On Diff #191676)

Rather than call the matcher (which is a heavy-handed solution), I'd prefer to just check Node.isOMPStructuredBlock() directly.

lebedev.ri marked 2 inline comments as done.

Last nit.

include/clang/ASTMatchers/ASTMatchers.h
6444–6445 ↗(On Diff #191676)

Hmm, k.

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