aaron.ballman added a comment.
Pragmas and magic comments both have ergonomic issues, so it's really about the
tradeoffs. Personally, I think it's more reasonable to support this as a pragma
than as a magic-comment. Pragmas are a mystery to users, but they're less of a
mystery than comments that carry semantic weight. I think it's somewhat more
advantageous to use a pragma because if a system header uses this pragma in an
unguarded way, they get told about their mistake (which is somewhat valuable),
and code editors are far better about autocompleting actual syntax than they
are with things like magic comments.
================
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">,
----------------
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).
================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:304
+def pp_pragma_include_instead_not_sysheader : Warning<
+ "'#pragma include_instead' ignored outside of system headers">,
+ InGroup<PragmaIncludeInstead>,
----------------
I think these diagnostics should say `'#pragma clang include_instead'` (we're
not super consistent about this, but the surrounding single quotes are intended
to convey syntactic constructs and so it should probably be valid syntax).
================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:306
+ InGroup<PragmaIncludeInstead>,
+ ShowInSystemHeader;
+def pp_pragma_include_instead_unexpected_token : Warning<
----------------
O.o... why do we need to show this one in system headers if the diagnostic can
only trigger outside of a system header?
================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:307-310
+def pp_pragma_include_instead_unexpected_token : Warning<
+ "'#pragma include_instead' expects '%0' as its next token; got '%1'
instead">,
+ InGroup<PragmaIncludeInstead>,
+ ShowInSystemHeader;
----------------
No need for a new warning -- I think we can use existing diagnostics for this
(like `err_pp_expects_filename`). Also, I think syntax issues should be
diagnosed as an error instead of a warning -- we expect a particular syntactic
form and it's reasonable for us to enforce that users get it right.
================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:311-314
+def pp_pragma_include_instead_file_not_found : Warning<
+ "'%0' file not found">,
+ InGroup<PragmaIncludeInstead>,
+ ShowInSystemHeader;
----------------
Do we need this one or can we use `err_pp_file_not_found`?
================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:315
+ ShowInSystemHeader;
+def pp_pragma_include_instead_system_reserved : Warning<
+ "header '%0' is an implementation detail; #include %select{'%2'|either '%2'
or '%3'|one of %2}1 instead">,
----------------
Do we want to give this pragma teeth by making this an error? Right now, an
intrepid user can simply ignore the diagnostic, form the reliance on the
header, and we've not really solved the problem we set out to solve. Then
again, perhaps you'd like that as an escape hatch for some reason?
================
Comment at: clang/include/clang/Lex/PreprocessorLexer.h:188-189
+
+ void addInclude(const StringRef Filename, const FileEntry &File,
+ const SourceLocation Location) {
+ IncludeHistory.insert({Filename, {&File, Location}});
----------------
It's not wrong, but we don't usually do top-level `const` qualification of
value types.
================
Comment at: clang/lib/Lex/PPDirectives.cpp:2025
+ // Record the header's filename for later use
+ if (File)
----------------
================
Comment at: clang/lib/Lex/PPLexerChange.cpp:313
+
+ auto IncludeHistory = CurLexer->getIncludeHistory();
+ for (const auto &Include : IncludeHistory) {
----------------
Please spell out the type instead of using `auto` or sink this into the `for`
loop
================
Comment at: clang/lib/Lex/PPLexerChange.cpp:315
+ for (const auto &Include : IncludeHistory) {
+ const StringRef Filename = Include.getKey();
+ const auto &IncludeInfo = Include.getValue();
----------------
================
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);
----------------
Please spell out the type.
================
Comment at: clang/lib/Lex/PPLexerChange.cpp:335
+ std::string Aliases;
+ llvm::interleave(
+ Info.Aliases,
----------------
Can you use `llvm::join()` from `StringExtras.h` for this or does the insertion
of the single quotes make that awkward?
================
Comment at: clang/lib/Lex/PPLexerChange.cpp:337
+ Info.Aliases,
+ [&Aliases](const StringRef S) { Aliases += ("'" + S + "'").str(); },
+ [&Aliases] { Aliases += ", "; });
----------------
================
Comment at: clang/lib/Lex/Pragma.cpp:506
+ if (PP.LexHeaderName(FilenameTok, /*AllowConcatenation*/ false))
+ return {};
----------------
Please return `llvm::None` in all these cases to make it more clear what's
going on.
================
Comment at: clang/lib/Lex/Pragma.cpp:557
+ if (Tok.isNot(tok::l_paren)) {
+ const std::string spelling = getSpelling(Tok);
+ Diag(Tok, diag::pp_pragma_include_instead_unexpected_token)
----------------
================
Comment at: clang/lib/Lex/Pragma.cpp:566-568
+ if (!FilenameTok) {
+ return;
+ }
----------------
================
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)
----------------
================
Comment at: clang/lib/Lex/Pragma.cpp:588-590
+ if (!FilenameTok) {
return;
}
----------------
================
Comment at: clang/lib/Lex/Pragma.cpp:1082
+/// PragmaIncludeInsteadHandler - "\#pragma include_instead(header)" marks the
+/// current file as non-includable if the including header is not a system
----------------
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
