ilya-biryukov added inline comments.
================ Comment at: include/clang/Lex/Preprocessor.h:391 } PreambleConditionalStack; + bool PreambleGenerationFailed = false; ---------------- 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? ================ Comment at: lib/Lex/PPDirectives.cpp:1945 + // Generate a fatal error to skip further processing. + Diag(FilenameTok.getLocation(), diag::err_pp_error_opening_file) << "" + << ""; ---------------- nik wrote: > ilya-biryukov wrote: > > Maybe add a new error or find a more appropriate existing one to reuse? > > "Error opening file", especially without any arguments does not provide > > enough context to figure out what went wrong. > > Maybe add a new error or find a more appropriate existing one to reuse? > > As stated above, introducing a new diagnostic that the user will never face > feels wrong, but that's just a preference. If you prefer a dedicated > diagnostics, that's fine for me. > > Checking the existing ones, there are not many fatal errors to choose from: > > err_pp_file_not_found > err_pp_through_header_not_found > err_pp_through_header_not_seen > err_pp_pragma_hdrstop_not_seen > err_pp_error_opening_file > > The last one looks the best for me. > > > "Error opening file", especially without any arguments does not provide > > enough context to figure out what went wrong. > > I've added some arguments which might be useful for debugging. Could we create a new error for this case? ================ Comment at: lib/Lex/PPDirectives.cpp:1946 + Diag(FilenameTok.getLocation(), diag::err_pp_error_opening_file) + << Filename << "Main file can't include itself for preamble."; + return; ---------------- I thought the convention is to **not** capitalize the first word of a message, otherwise it would be a bit inconsistent with other errors: ``` cannot open file 'foo.cpp: Main file can't include itself for preamble ``` also a small nit: maybe rephrase to `main file cannot be included recursively for preamble` and another small nit: we probably want to remove the full stop at the end. 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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
