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