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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
