[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

[Aaron Ballman via Phabricator via cfe-commits]

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