Defer addition of keywords to identifier table when loading AST
ClosedPublic

Authored by jklaehn on Jul 9 2017, 9:10 AM.

Details

Summary

In ASTUnit::LoadFromASTFile, the preprocesor object is set up using
default-constructed LangOptions (which only later get populated).
Then, in the constructor of IdentifierTable, these default-constructed
LangOptions were used in the call to AddKeywords, leading to wrong
initialization of the identifier table.

This change defers adding the keywords to the identifier table until
after the language options have been loaded from the AST file.

Prior to this change the included test would fail due to the class
token being reported as an identifier (since the C++11 enum class
construct is not present when using the default language options).

Diff Detail

jklaehn created this revision.Jul 9 2017, 9:10 AM

Thanks for the patch!

lib/Frontend/ASTUnit.cpp
541 ↗(On Diff #105781)

Have you tried adding the keywords in PP.Initialize? We might not need an additional parameter to the constructor then.

jklaehn updated this revision to Diff 123495.Nov 19 2017, 4:58 AM
jklaehn added a reviewer: arphaman.

Thanks for taking a look! I removed the constructor argument as suggested; keywords are now added in PP.Initialize.

Ping, can you take another look?

rsmith added a comment.Dec 7 2017, 3:06 PM

LGTM, but I'd like the old IdentifierTable constructor to be removed if there are no callers left.

include/clang/Basic/IdentifierTable.h
473–476

Can this constructor be removed?

LGTM, but I'd like the old IdentifierTable constructor to be removed if there are no callers left.

It's still being used in e.g. FormatTokenLexer, where the populated IdentifierTable is passed to the constructor of another member:

FormatTokenLexer::FormatTokenLexer(const SourceManager &SourceMgr, FileID ID,
                                   unsigned Column, const FormatStyle &Style,
                                   encoding::Encoding Encoding)
    : ..., IdentTable(getFormattingLangOpts(Style)),
      Keywords(IdentTable), ... {
struct AdditionalKeywords {
  AdditionalKeywords(IdentifierTable &IdentTable) {
    kw_final = &IdentTable.get("final");
    ...

Apart from this case (for which I would opt to keep the old constructor) there are three other uses which could easily be changed to the new signature.
Would you prefer to land this change with the old constructor in place or should I make the required changes to remove it?

jklaehn updated this revision to Diff 127685.Dec 20 2017, 4:54 AM
jklaehn added a project: Restricted Project.

ping (rebased)

jklaehn updated this revision to Diff 141033.Apr 4 2018, 1:23 PM
jklaehn set the repository for this revision to rC Clang.

ping? (rebased)

friendly ping? :)

aaron.ballman accepted this revision.Apr 14 2018, 7:02 AM
aaron.ballman added a subscriber: aaron.ballman.

LGTM aside from a few small nits.

include/clang/Basic/IdentifierTable.h
471

ExternalLookup to match coding conventions. Also, can you mark this constructor as explicit? Might as well change the other constructor as well since it's being reformatted.

This revision is now accepted and ready to land.Apr 14 2018, 7:02 AM
jklaehn updated this revision to Diff 142579.Apr 15 2018, 10:39 AM

Thanks for the review! As I do not have commit access, it would be great if you could commit the updated patch.

aaron.ballman closed this revision.Apr 16 2018, 2:10 PM

Committed in r330159. Thank you for the patch!