[PATCH] D53866: [Preamble] Stop generating preamble for circular #includes

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: include/clang/Basic/DiagnosticLexKinds.td:432
+def err_pp_including_mainfile_for_preamble : Error<
+  "main file cannot be included recursively for preamble">, DefaultFatal;
 def err_pp_empty_filename : Error<"empty filename">;
----------------
NIT: maybe rephrase `for preamble` to `when building a preamble`? "for preamble" might be a bit confusing.


================
Comment at: include/clang/Lex/Preprocessor.h:391
   } PreambleConditionalStack;
+  bool PreambleGenerationFailed = false;
 
----------------
nik wrote:
> ilya-biryukov wrote:
> > nik wrote:
> > > ilya-biryukov wrote:
> > > > There's a mechanism to handle preamble with errors, see `PreprocessorOpts::AllowPCHWithCompilerErrors`.
> > > > Maybe rely on it and avoid adding a different one?
> > > I'm not sure how to make use of AllowPCHWithCompilerErrors. It's clearly meant as an input/readonly option - do you suggest setting that one to "false" in case we detect the cyclic include (and later checking for it...)? That feels a bit hacky. Have you meant something else?
> > We emit an error, so the preamble will **not** be emitted. 
> > Unless the users specify `AllowPCHWithCompilerErrors`, in which case they've signed up for this anyway.
> > 
> > I propose to **not** add the new flag at all. Would that work?
> As stated further above, no.
> 
> That's because for the libclang/c-index-test case, AllowPCHWithCompilerErrors=true - see clang_parseTranslationUnit_Impl. As such, the preamble will be generated/emitted as the second early return in PCHGenerator::HandleTranslationUnit is never hit.
if `AllowPCHWithCompilerErrors=true` is set to true, why not simply pretend the include was not found? That would still generate the preamble, but users have the error message to see that something went wrong.

`c-index-test` produces the errors, so we can check the error is present in the output.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53866