[PATCH] D53866: [Preamble] Fix preamble for circular #includes

[Nikolai Kosjar via Phabricator via cfe-commits]

nik marked 2 inline comments as done.
nik added inline comments.


================
Comment at: include/clang/Lex/Preprocessor.h:391
   } PreambleConditionalStack;
+  bool PreambleGenerationFailed = false;
 
----------------
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?


================
Comment at: lib/Lex/PPDirectives.cpp:1945
+    // Generate a fatal error to skip further processing.
+    Diag(FilenameTok.getLocation(), diag::err_pp_error_opening_file) << ""
+                                                                     << "";
----------------
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53866