Page MenuHomePhabricator

Allow for unfinished #if blocks in preambles.
ClosedPublic

Authored by erikjv on Jan 8 2016, 6:06 AM.

Details

Summary

Previously, a preamble only included #if blocks (and friends like
ifdef) if there was a corresponding #endif before any declaration or
definition. The problem is that any header file that uses include guards
will not have a preamble generated, which can make code-completion very
slow.

To prevent errors about unbalanced preprocessor conditionals in the
preamble, and unbalanced preprocessor conditionals after a preamble
containing unfinished conditionals, the conditional stack is stored
in the pch file.

This fixes PR26045.

Diff Detail

Repository
rL LLVM

Event Timeline

erikjv updated this revision to Diff 44321.Jan 8 2016, 6:06 AM
erikjv retitled this revision from to Allow for unfinished #if blocks in preambles..
erikjv updated this object.
erikjv added a reviewer: bkramer.
erikjv added a subscriber: cfe-commits.
bkramer accepted this revision.Jan 13 2016, 7:17 AM
bkramer edited edge metadata.

After pondering this patch for a while I couldn't come up with a case where this would cause problems, but it provides great improvements for editing headers in an interactive setup. So LGTM from my side.

This revision is now accepted and ready to land.Jan 13 2016, 7:17 AM
erikjv abandoned this revision.Jun 7 2016, 7:26 AM

This will give errors about unbalanced #if/#endif for header guards.

erikjv reclaimed this revision.Jul 5 2016, 7:29 AM
This revision is now accepted and ready to land.Jul 5 2016, 7:29 AM
erikjv updated this revision to Diff 62748.Jul 5 2016, 7:33 AM
erikjv updated this object.
erikjv edited edge metadata.

This version stores/loads the conditional stack in the preprocessor the the generated pch file. It suppresses errors about unfinished conditional preprocessor blocks, and after restoring, it will check if the block is closed in the main file.

Looks like patch was not committed.

Yes, the patch was not committed, because there were errors in it. So, I fixed it and put up a new version, but I have no idea how to reset the state.

Still waiting for rsmith to review...

rsmith added inline comments.Oct 26 2016, 1:09 PM
include/clang/Lex/Preprocessor.h
283–286 ↗(On Diff #62748)

What's the difference between the Off and Done states? They seem to be the same to me?

317 ↗(On Diff #62748)

{ on previous line.

326 ↗(On Diff #62748)

Start function names with a lowercase letter.

329 ↗(On Diff #62748)

We don't really care much about the Preprocessor object getting a few dozen bytes larger, since there will typically only be one of them. I don't think the complexity of a dynamically-allocated stack and a PointerIntPair is worthwhile here. Just store a Stack and a State directly as members.

1959–1963 ↗(On Diff #62748)

Start function names with a lowercase letter.

lib/Lex/Lexer.cpp
622 ↗(On Diff #62748)

You can remove the special-case handling for #if/#else/#endif here.

2529–2532 ↗(On Diff #62748)

We should only do this if we reach the end of the main source file. If we reach the end of a #include'd file with a #if still open, we should diagnose that.

lib/Serialization/ASTReader.cpp
2816 ↗(On Diff #62748)

Why can't we set the conditional stack on the PPLexer directly from here? (Why do we need to store it separately?)

erikjv marked 7 inline comments as done.Dec 7 2016, 6:59 AM
erikjv added inline comments.
lib/Serialization/ASTReader.cpp
2816 ↗(On Diff #62748)

Because there is no PPLexer yet. The deserialization is kicked off by ASTUnit::CodeComplete in Act->BeginSourceFile, while the lexer is created in Act->Execute.

erikjv updated this revision to Diff 80589.Dec 7 2016, 7:01 AM
rsmith added inline comments.Feb 7 2017, 2:04 PM
include/clang/Lex/Preprocessor.h
310 ↗(On Diff #80589)

Return an ArrayRef rather than exposing the type of the storage (which is an implementation detail), here and transitively through the callers of this.

322 ↗(On Diff #80589)

Please pass in an ArrayRef here rather than a SmallVector.

328 ↗(On Diff #80589)

I don't see a need to heap-allocate this separately from the Preprocessor.

include/clang/Lex/PreprocessorOptions.h
84 ↗(On Diff #80589)

Please add a doc comment for this option. Also, maybe GeneratePreamble to avoid ambiguity as to whether this is some kind of generation number for the preamble?

lib/Lex/PPLexerChange.cpp
139–142 ↗(On Diff #80589)

I think this would make more sense two callers up, in Preprocessor::EnterMainSourceFile

erikjv updated this revision to Diff 87792.Feb 9 2017, 4:27 AM
erikjv marked 5 inline comments as done.

Thanks for working on this! I have some comments below:

include/clang/Lex/Preprocessor.h
291 ↗(On Diff #87792)

setState is redundant IMHO, just assign to ConditionalStackState when you use setState below.

301 ↗(On Diff #87792)

the const is redundant for the return type.

310 ↗(On Diff #87792)

You can pass ArrayRef by value.

1974 ↗(On Diff #87792)

the const is redundant for the return type.

1978 ↗(On Diff #87792)

Please use ArrayRef instead of SmallVector here and in the function below.

include/clang/Lex/PreprocessorLexer.h
181 ↗(On Diff #87792)

ArrayRef can be passed by value.

include/clang/Lex/PreprocessorOptions.h
87 ↗(On Diff #87792)

Typo: save

lib/Serialization/ASTReader.cpp
2896 ↗(On Diff #87792)

Loc should be capitalized.

lib/Serialization/ASTWriter.cpp
2269 ↗(On Diff #87792)

I think you should add an assertion here that verifies that the ASTWriter isn't creating a module.

erikjv marked 8 inline comments as done.May 12 2017, 6:51 AM

Fixed with the next patch.

erikjv updated this revision to Diff 98757.May 12 2017, 6:53 AM
nik added a subscriber: nik.May 17 2017, 11:45 PM

Ping?!

rsmith accepted this revision.May 18 2017, 4:22 PM

Some comments, but I'm happy for you to go ahead and commit after addressing them. Thanks!

include/clang/Lex/Preprocessor.h
2004 ↗(On Diff #98757)

Please put the { on the previous line and the } on the next line. We don't use this "sandwich braces" style in clang except when defining the function on the same line as the declaration.

include/clang/Lex/PreprocessorLexer.h
182 ↗(On Diff #98757)

{ on the previous line, please.

lib/Lex/Lexer.cpp
2548 ↗(On Diff #98757)

This should be in the isInPreamble() conditional below, too. We don't want to make a redundant copy of the ConditionalStack at the end of each #included file, just at the end of the overall preamble.

lib/Lex/PPLexerChange.cpp
49 ↗(On Diff #98757)

I think this would be better named as isInMainFile: we don't care whether we're in the preamble, or even if there is one, here (the caller checks that).

This revision was automatically updated to reflect the committed changes.
erikjv marked 3 inline comments as done.