This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Fix duplicate class template definitions problem
ClosedPublic

Authored by martong on May 16 2018, 8:23 AM.

Details

Summary

We fail to import a ClassTemplateDecl if the "To" context already contains a definition and then a forward decl.
This is because localUncachedLookup does not find the definition.
This is not a lookup error, the parser behaves differently than assumed in the importer code.
A DeclContext contains one DenseMap (LookupPtr) which maps names to lists.
The list is a special list StoredDeclsList which is optimized to have one element.
During building the initial AST, the parser first adds the definition to the DeclContext.
Then during parsing the second declaration (the forward decl) the parser again calls DeclContext::addDecl but that will not add a new element to the StoredDeclsList rarther it simply overwrites the old element with the most recent one.
This patch fixes the error by finding the definition in the redecl chain.
Added tests for the same issue with CXXRecordDecl and with ClassTemplateSpecializationDecl.
These tests pass and they pass because in VisitRecordDecl and in VisitClassTemplateSpecializationDecl we already use D->getDefinition() after the lookup.

Diff Detail

Repository
rC Clang

Event Timeline

martong created this revision.May 16 2018, 8:23 AM
martong retitled this revision from Fix duplicate class template definitions problem to [ASTImporter] Fix duplicate class template definitions problem.May 16 2018, 8:56 AM

Hello Gabor,

The patch is fine, I have found only some style issues.

lib/AST/ASTImporter.cpp
4073

forward

4121–4127

is a definition?

4124

let's; if it is available

4126
  • is misplaced here.
unittests/AST/ASTImporterTest.cpp
1550

Although long names make me happy, this one exceeds 80-char limit. We can abbreviate "Specialization" to "Spec".

unittests/AST/DeclMatcher.h
52

Predicate, Count (member names should start with capital).

martong updated this revision to Diff 148013.May 22 2018, 8:25 AM
martong marked 5 inline comments as done.
  • Addressing Aleksei's comments
martong marked an inline comment as done.May 22 2018, 8:26 AM
a.sidorin accepted this revision.May 22 2018, 9:23 AM

LGTM, thanks!

This revision is now accepted and ready to land.May 22 2018, 9:23 AM
This revision was automatically updated to reflect the committed changes.

Hm. Should we test -fms-compatibility in addition to -fdelayed-template-parsing?

@a.sidorin
Yes, that is a very good idea.
Just created a new patch which adds that switch for the tests which use the TestBase.
https://reviews.llvm.org/D47367