Page MenuHomePhabricator

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

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

Diff Detail


Event Timeline

lebedev.ri created this revision.Mar 16 2019, 11:16 AM
aaron.ballman added inline comments.Mar 16 2019, 11:51 AM
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)?

513–515 ↗(On Diff #190982)

Alphabetical order, please.

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


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


<code snippet>

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


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

<code snippet>
For example:


   #pragma omp parallel default(none)
   #pragma omp parallel default(shared)
   #pragma omp parallel

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

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.

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