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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D61509#1491158 <https://reviews.llvm.org/D61509#1491158>, @jdenny wrote:

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


Could work. And then move `PragmaIntroducerKind` into it.
I believe this part of the refactoring is completely uncontroversial.

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

I'm not sure either one is better than the other one.

I have two concerns:

- I fear this would result in inconsistency with other pragmas, since this will only change openmp-ones. I don't know if it will be accepted to migrate the rest of them in the same way.
- This use-case requires having the location of `#pragma`, so the entire AST is migrating to store it. But the current location will no longer be accessible from AST.

I see two paths forward:

- Mail cfe-dev, and suggest to do this change for *all* pragmas. Either this is ok for all of them, or none of them.
- Moar abstractions - how about **not** changing the startloc of openmp directives, but instead add some baseclass to `OMPDirective` class (& every other class that is created from pragma), that would record the `PragmaIntroducer`?

> 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