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

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

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

> In D61509#1491397 <https://reviews.llvm.org/D61509#1491397>, @jdenny wrote:
>
> > In D61509#1491209 <https://reviews.llvm.org/D61509#1491209>, @ABataev wrote:
> >
> > > In D61509#1491204 <https://reviews.llvm.org/D61509#1491204>, @lebedev.ri wrote:
> > >
> > > > 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,
> > >
> >
> >
> > My alternative proposal was exactly that.  A difficulty is how to pass the `#pragma` location to the OpenMP AST node constructors.  `PragmaOpenMPHandler::HandlePragma` passes locations via the `tok::annot_pragma_openmp` and `tok::annot_pragma_openmp_end` tokens, so where do we pass this new location?  I proposed creating a third token, and Alexey was concerned over the parsing problems that would create.
> >
> > >>   but instead add some baseclass to `OMPDirective` class (& every other class that is created from pragma), that would record the `PragmaIntroducer`?
> >
> > I agree that a base class would be a nice way to extend all pragma classes with the `PragmaIntroducer`.
> >
> > > I'm against this solution. I don't see any reasons why we should do this. Instead, we're getting a lot of pain with parsing and maintenance.
> >
> > One way to avoid creating an extra token would be to widen the `Token` class to store the additional location.  The `Token` documentation says it's not intended to be space efficient.  How does that sound to people?
>
>
> That was my proposal, yes.


@ABataev : Does that address your concerns?


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

https://reviews.llvm.org/D61509