[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

Sun May 5 08:05:14 PDT 2019

jdenny added a comment.

In D61509#1491119 <https://reviews.llvm.org/D61509#1491119>, @lebedev.ri wrote:

> I recommend to split this into two parts - changing `PragmaIntroducerKind Introducer` to
>  `struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};`, and the actual openmp change.

Sure, I'll work on that.  What about NameMe = PragmaIntroducer?

> For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the `#pragma` in their locations?  If so, why is that?

The other variant I originally proposed doesn't alter existing locations and so wouldn't affect diagnostics, but Alexey pointed out potential issues for changes it requires in the parser.

> so i'd like to see the proposed use-case.

That's a bit tricky.  I'll explain.

My use case right now is not for OpenMP locations but for OpenACC locations in our Clacc project, which is not upstream.  Clacc is calling Rewriter and passing locations from the OpenACC AST in order to rewrite the OpenACC pragmas to OpenMP.  Without the location for an OpenACC pragma's `#pragma`, Clacc cannot easily remove an OpenACC pragma or insert code before it.  At least not that I found.  Of course, I'd be happy for someone to explain to me how Rewriter was designed to be used differently.  I'd also be happy to share relevant code samples from Clacc if that would help.

So what does this have to do with OpenMP locations?  I'm anticipating that someone will one day want to use Rewriter in this way with other pragmas, so I'm offering some scaffolding toward that goal for all pragmas, and I'm completing the fix only for OpenMP, which is the existing pragma set I understand best.  Moreover, offering this now facilitates keeping Clacc up to date with master and eventually considering it for upstreaming.

Thanks for your responses.

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

https://reviews.llvm.org/D61509