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

Mon May 6 07:02:32 PDT 2019

ABataev added a comment.

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?

Again, I don't see a single point why would we need this. I don't think there is a big difference between the location of pragma keyword and `omp` keyword.

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

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

Annotation tokens already have extra locations. Do you need the third one? Plus, how you're going to store this location in the AST? We don't need any extra base classes. If you want to store this in AST nodes, add a new field to the existing classes and extended constructors/Create/(de-)serialization functions.

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

https://reviews.llvm.org/D61509