kristina added a comment.
Have you tested that build is successful and obviously the FileCheck test
should pass now? What sort of behavior does this produce when you attempt to
use an invalid path like that now when compiling with clang? Just a predecessor
diagnostic?
I'm updating software on my buildbot so I can't really do a quick run to test
it right now, though if you definitely know it compiles and the test passes,
I'm happy to approve this.
================
Comment at: lib/Lex/PPDirectives.cpp:1892
if (!File) {
- while (!isAlphanumeric(Filename.front())) {
+ while (!Filename.empty() && !isAlphanumeric(Filename.front())) {
Filename = Filename.drop_front();
----------------
hokein wrote:
> 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").
>
Hm, I think it's up to you if you want to make a new diagnostic or reuse one
that's similar enough, as long as you emit it and then bail out before you hit
the assertion, that should fix the bug. I like the new asserts as well.
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
