Page MenuHomePhabricator

[preamble] Also record the "skipping" state of the preprocessor
ClosedPublic

Authored by erikjv on Oct 5 2017, 3:53 AM.

Details

Reviewers
ilya-biryukov
Summary

When a preamble ends in a conditional preprocessor block that is being
skipped, the preprocessor needs to continue skipping that block when
the preamble is used.

This fixes PR34570.

Diff Detail

Event Timeline

erikjv created this revision.Oct 5 2017, 3:53 AM
ilya-biryukov accepted this revision.Oct 6 2017, 2:58 AM

LGTM. Only a few code style-related comments.

include/clang/Lex/Preprocessor.h
102

Maybe rename to HashTokenLoc for consistencty with IfTokenLoc?

337

Maybe move closer to other fields to avoid mixing data and functions?

1863

Maybe rename to HashTokenLoc for consistencty with IfTokenLoc?

lib/Lex/PPDirectives.cpp
353

Maybe rename to HashTokenLoc for consistencty with IfTokenLoc?

This revision is now accepted and ready to land.Oct 6 2017, 2:58 AM
ilya-biryukov requested changes to this revision.Oct 6 2017, 3:21 AM

Also, one more important thing. I'll unaccept revision for now, before getting feedback on this one.

include/clang/Lex/Preprocessor.h
337

Maybe initialize SkipInfo with default values? Or even better: forbid default constructor and put it into llvm::Optional?

It's a public member, so I guess there's a huge potential some code will eventually get into undefined behavior when using it.

This revision now requires changes to proceed.Oct 6 2017, 3:21 AM
erikjv updated this revision to Diff 117986.Oct 6 2017, 6:19 AM
erikjv edited edge metadata.
erikjv marked 5 inline comments as done.
This revision is now accepted and ready to land.Oct 6 2017, 6:23 AM
nik added a subscriber: nik.Oct 16 2017, 11:47 PM

This fixes the reported bug for me :)

There is another related issue that is not addressed by this change. I've reported it as

[preamble] Skipped ranges vanish after reparse (#ifdef with #include)
https://bugs.llvm.org/show_bug.cgi?id=34971

cameron314 added a subscriber: cameron314.EditedOct 19 2017, 11:19 AM

@nik, pretty sure this fix I submitted ages ago fixes that. Been using it in production a couple years. Would be nice if someone reviewed it so we could finally upstream it.

erikjv closed this revision.Nov 28 2017, 3:12 AM

Submitted as r317308.