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