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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:304
+
+def pp_pragma_include_instead_not_sysheader : Error<
+  "'#pragma clang include_instead' cannot be used outside of system headers">,
----------------
Can you prefix the diagnostic identifiers with `err_`? We're not super consistent in this file, but it's helpful when reading the use of the diagnostic to know whether it's an error or not.


================
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">,
----------------
cjdb wrote:
> 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.
I would prefer errors here -- the goal of this feature is to prevent programmers from doing things we know are going to be a maintenance burden, so I think we should be restrictive here. If users find the restrictions too onerous and have a good reason for us to relax them, we can definitely consider it then.


================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:306
+  InGroup<PragmaIncludeInstead>,
+  ShowInSystemHeader;
+def pp_pragma_include_instead_unexpected_token : Warning<
----------------
cjdb wrote:
> 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.
You can remove the `ShowInSystemHeader` bits now because these are all errors (at least, I'm 99% sure you can remove it).


================
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;
----------------
cjdb wrote:
> aaron.ballman wrote:
> > 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.
> Hmm... the syntax is `#pragma clang include_instead(header)`. This is diagnosing the lack of `(` or `)`, not a missing filename. Perhaps there's already a diagnostic for that?
There is: `diag::err_expected` (and some related ones in DiagnosticCommonKinds.td).


================
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);
----------------
cjdb wrote:
> 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`?
`PreprocessorLexer` already friends `Preprocessor`, so you can name it directly here (though please change the local variable name to be something different from the type name while you're at it).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106394/new/

https://reviews.llvm.org/D106394