[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/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