[PATCH] D106394: [clang][pp] adds '#pragma include_instead'
Christopher Di Bella via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 22 13:28:09 PDT 2021
cjdb added a comment.
Thanks for the first pass @aaron.ballman! Some clarifying questions, but I think I have enough of your other comments to work on while you ponder these.
================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:303
InGroup<DiagGroup<"pragma-system-header-outside-header">>;
+def pp_pragma_include_instead_not_sysheader : Warning<
+ "'#pragma include_instead' ignored outside of system headers">,
----------------
aaron.ballman wrote:
> Design-wise, do we want this one to be an error instead of a warning? I don't have strong feelings one way or the other, but making it an error ensures no one finds creative ways to use it outside of a system header by accident (and we can relax the restriction later if there are reasons to do so).
I'd prefer it to be an error, but I wasn't confident that would be accepted (same applies to all your comments re warn vs err). If you're game, I'll make the change.
================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:306
+ InGroup<PragmaIncludeInstead>,
+ ShowInSystemHeader;
+def pp_pragma_include_instead_unexpected_token : Warning<
----------------
aaron.ballman wrote:
> O.o... why do we need to show this one in system headers if the diagnostic can only trigger outside of a system header?
Mostly because I'm scared that there's some toggle that would accidentally make the diagnostic observable in a system header and wanted to test the behaviour.
================
Comment at: clang/lib/Lex/PPLexerChange.cpp:316
+ const StringRef Filename = Include.getKey();
+ const auto &IncludeInfo = Include.getValue();
+ const HeaderFileInfo &Info = HeaderInfo.getFileInfo(IncludeInfo.File);
----------------
aaron.ballman wrote:
> Please spell out the type.
The type's currently a private type (that was by design). Do you want me to make it a public type or pull it out into `namespace clang`?
================
Comment at: clang/lib/Lex/Pragma.cpp:559
+ Diag(Tok, diag::pp_pragma_include_instead_unexpected_token)
+ << "(" << (spelling == "\n" ? "\\n" : spelling);
+ return;
----------------
I thought I deleted this.
================
Comment at: clang/lib/Lex/Pragma.cpp:572
+ if (Tok.isNot(tok::r_paren)) {
+ const std::string spelling = getSpelling(Tok);
+ Diag(Tok, diag::pp_pragma_include_instead_unexpected_token)
----------------
aaron.ballman wrote:
>
Alternatively: remove `spelling` altogether.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106394/new/
https://reviews.llvm.org/D106394
More information about the cfe-commits
mailing list