ABataev added a comment. 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, 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
