[PATCH] D52721: [Preprocessor] Fix a crash when handling non-alpha include header.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: lib/Lex/PPDirectives.cpp:1891
       StringRef OriginalFilename = Filename;
       if (!File) {
+        while (!Filename.empty() && !isAlphanumeric(Filename.front())) {
----------------
sammccall wrote:
> everything in this block is guarded by !Filename.empty().
> Just add it to the if condition?
The logic is tricky... inside the if body, we modify `Filename`, so we can't just add the judgement here.


================
Comment at: lib/Lex/PPDirectives.cpp:1892
       if (!File) {
-        while (!isAlphanumeric(Filename.front())) {
+        while (!Filename.empty() && !isAlphanumeric(Filename.front())) {
           Filename = Filename.drop_front();
----------------
kristina wrote:
> kristina wrote:
> > This line is tripping the assert, it seems best course of action would be a single check here and then just diagnosing an error unless you have managed to find other cases, in which case all the checks below are also warranted.
> In either case, the diagnostic emitted doesn't really make sense, at least to me, I think it would be better to explicitly diagnose this case as an error and then bail, before an assertion fires.
> 
> I also crashed clang with `#include "./"`, so the test case does seem to be fairly minimal which is good. Though I think a diagnostic about a bogus file path would be better (I don't know how to word it well), rather than saying file not found.
Thanks for the comment.

IIUC, do you mean we create a new diagnostic message for this specific case? I'm double that it is worth doing it, since IMO this is a heuristic that getting a typo file path, and user input is arbitrary, make the diagnostic fit all users input would be very tricky.

I think we can just provide best-effort for this case (e.g. no crash), "err_pp_file_not_found" seems the best one from existing messages. 

I checked with `g++,` it provides a slightly better message ("fatal error: ./: No such file or directory" vs clang's "'./' file not found").



================
Comment at: lib/Lex/PPDirectives.cpp:1925
 
+  if (Filename.empty())
+    return;
----------------
sammccall wrote:
> this is mysterious - what does it solve? is it the right place to handle this problem?
> 
> (You allude to a code complete crash - can you explain?)
ah, I created this patch when fixing a clangd crash, it seems still crash clangd without it, but we can pass the test without it. I think that is another issue, so I remove this code now, and address it in a follow-up patch.


Repository:
  rC Clang

https://reviews.llvm.org/D52721