This is an archive of the discontinued LLVM Phabricator instance.

[PragmaHandler] Expose `#pragma` location
ClosedPublic

Authored by jdenny on May 7 2019, 9:41 AM.

Details

Summary

Currently, a pragma AST node's recorded location starts at the
namespace token (such as omp in the case of OpenMP) after the
#pragma token, and the #pragma location isn't available. However,
the #pragma location can be useful when, for example, rewriting a
directive using Clang's Rewrite facility.

This patch makes #pragma locations available in any PragmaHandler
but it doesn't yet make use of them.

This patch also uses the new struct PragmaIntroducer to simplify
Preprocessor::HandlePragmaDirective. It doesn't do the same for
PPCallbacks::PragmaDirective because that changes the API documented
in clang-tools-extra/docs/pp-trace.rst, and I'm not sure about
backward compatibility guarantees there.

This patch was forked from D61509.

Diff Detail

Repository
rC Clang

Event Timeline

jdenny created this revision.May 7 2019, 9:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2019, 9:41 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript

Seems like obvious NFC to me, but i'm not sure about API/ABI implications for external use.

clang/docs/ClangPlugins.rst
58 ↗(On Diff #198489)

Hmm.
This will have effects on out-of-tree plugins that define pragmas.
I'm not sure how to proceed here, just notify cfe-dev and move on?

jdenny marked an inline comment as done.May 7 2019, 10:40 AM
jdenny added inline comments.
clang/docs/ClangPlugins.rst
58 ↗(On Diff #198489)

We could do something like this in PragmaHandler:

virtual void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer,
                          Token &FirstToken) {
  llvm_unreachable("deprecated HandlePragma unexpectedly called");
}
virtual void HandlePragma(Preprocessor &PP, PragmaIntroducer Introducer,
                          Token &FirstToken) {
  HandlePragma(PP, Introducer.Kind, FirstToken);
}

The derived class could override either one. Unfortunately, if it didn't override either, the error is then at run time instead of compile time as it is now.

Whether this is worth it, I don't know. Who's the right person to answer this question?

lebedev.ri added inline comments.May 14 2019, 3:00 AM
clang/docs/ClangPlugins.rst
58 ↗(On Diff #198489)

I think mailing cfe-dev will get this question the most visibility.
Though i think the solution will be to go with the current patch.

jdenny marked an inline comment as done.May 21 2019, 7:24 AM
jdenny added inline comments.
clang/docs/ClangPlugins.rst
58 ↗(On Diff #198489)

I tried that at

http://lists.llvm.org/pipermail/cfe-dev/2019-May/062297.htm

but it's been a week without a response. I'm happy to try again, perhaps with a shorter more general email about PragmaHandler backward compatibility that might catch a different set of eyes. Is it worth it, or should we just assume no response so far means no one thinks it's an issue? Thanks.

jdenny marked an inline comment as done.May 21 2019, 7:27 AM
jdenny added inline comments.
clang/docs/ClangPlugins.rst
58 ↗(On Diff #198489)

Sorry, copy and paste error. The URL is:

http://lists.llvm.org/pipermail/cfe-dev/2019-May/062297.html

aaron.ballman accepted this revision.May 21 2019, 7:54 AM

LGTM!

clang/docs/ClangPlugins.rst
58 ↗(On Diff #198489)

I don't think we make guarantees about the stability of these APIs and this is a loud break for out-of-tree users rather than a silent behavioral change. I think the approach taken in this patch is fine.

This revision is now accepted and ready to land.May 21 2019, 7:54 AM
lebedev.ri accepted this revision.May 21 2019, 8:14 AM

No one raised any concerns, so let's do this.

clang/docs/ClangPlugins.rst
58 ↗(On Diff #198489)

Indeed, i was waiting a bit of time since that post.
Since there has been no response..

jdenny marked an inline comment as done.May 21 2019, 8:34 AM
jdenny added inline comments.
clang/docs/ClangPlugins.rst
58 ↗(On Diff #198489)

Thanks, guys! I'll work on pushing soon.

jdenny retitled this revision from [PragmaHandler][NFC] Expose `#pragma` location to [PragmaHandler] Expose `#pragma` location.May 21 2019, 8:45 AM
This revision was automatically updated to reflect the committed changes.