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

[Nikolai Kosjar via Phabricator via cfe-commits]

nik marked 2 inline comments as done.
nik added inline comments.
Herald added a subscriber: jdoerfert.
Herald added a project: clang.


================
Comment at: include/clang/Lex/Preprocessor.h:391
   } PreambleConditionalStack;
+  bool PreambleGenerationFailed = false;
 
----------------
ilya-biryukov wrote:
> 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.
> if AllowPCHWithCompilerErrors=true is set to true, why not simply pretend the include was not found?

Sounds good, especially that the preamble is still generated. 

> That would still generate the preamble, but users have the error message to see that something went wrong.

The vanilla diagnostic in this case might be a bit confusing, I kept the introduced diagnostics as it carries more information.



Repository:
  rC Clang

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

https://reviews.llvm.org/D53866