Page MenuHomePhabricator

[Preprocessor] Stop entering included files after hitting a fatal error.
ClosedPublic

Authored by vsapsai on Jun 29 2018, 12:13 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

vsapsai created this revision.Jun 29 2018, 12:13 PM
jkorous added a subscriber: jkorous.
jkorous added inline comments.
clang/lib/Lex/PPDirectives.cpp
1875 ↗(On Diff #153553)

I am not sure I understand this - does that mean that we are not displaying diagnostics that were produced before "now"?

vsapsai added inline comments.Jul 18 2018, 4:14 PM
clang/lib/Lex/PPDirectives.cpp
1875 ↗(On Diff #153553)

I can see how the wording can cause the confusion. But I'm not entirely sure it is misguiding, I've copy-pasted it from Sema::InstantiatingTemplate::InstantiatingTemplate. Let me explain my reasoning in a different way to see if it makes sense. Entering a file is observable if it produces diagnostics or some other build artifact (object file in most cases). So when we encounter a fatal error, there is no visible indication of entering subsequent files. That's why we can skip entering those files: no difference in output and less work to do.

jkorous accepted this revision.Jul 19 2018, 4:01 AM

LGTM with just a nit about comment wording.

Thanks for the patch!

clang/lib/Lex/PPDirectives.cpp
1875 ↗(On Diff #153553)

Thanks for the explanation! I was not sure whether "only subsequent" OR "both subsequent and some/all prior" raised diagnostics would be not visible (could be just my shitty English though).

Maybe something along this line "any eventual subsequent diagnostics will not be visible" would be more clear?

This revision is now accepted and ready to land.Jul 19 2018, 4:01 AM
aaron.ballman accepted this revision.Jul 19 2018, 5:36 AM
aaron.ballman added a subscriber: aaron.ballman.

LGTM as well

bruno accepted this revision.Jul 19 2018, 5:49 AM

Thanks for working on this.

LGTM with improvements in the comments as mentioned by @jkorous

vsapsai added inline comments.Jul 23 2018, 3:39 PM
clang/lib/Lex/PPDirectives.cpp
1875 ↗(On Diff #153553)

Curiously, the comment kinda means both. Some prior diagnostics might be invisible but in this case we care about subsequent diagnostics. I'll update the comment to make that more clear.

vsapsai updated this revision to Diff 156911.Jul 23 2018, 4:02 PM
  • Tweak the comment according to review comments.

Undiscussed changes: we don't stop preprocessing entirely, only in included
files; not using better-sounding "skip" deliberately as it might be confused
with FileSkipped API.

This revision was automatically updated to reflect the committed changes.

Thanks everyone for reviewing the change.