lebedev.ri 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? That was my proposal, yes. ================ Comment at: clang-tools-extra/test/clang-tidy/openmp-exception-escape.cpp:1 // RUN: %check_clang_tidy %s openmp-exception-escape %t -- -extra-arg=-fopenmp=libomp -extra-arg=-fexceptions -config="{CheckOptions: [{key: openmp-exception-escape.IgnoredExceptions, value: 'ignored, ignored2'}]}" -- ---------------- Changes to this file will no longer be needed, i have changed the check. 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
