Skip to content
This repository has been archived by the owner on Mar 30, 2021. It is now read-only.

ASTImporter::Imported - VisitClassTemplateDecl assertion fix #316

Conversation

balazske
Copy link
Collaborator

@balazske balazske commented Mar 1, 2018

Fix for assertion
"Try to import an already imported Decl"
that occurs through calling
ASTNodeImporter::VisitClassTemplateDecl.

Fix for #312 .

Copy link

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, nice fix! Some nits inline.

TEST_F(Fixture,
ImportTemplatedDecl) {
auto Code =
R"(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code could be a one-line string literal rather than a multi-line raw string literal

template<class X> struct S{};
)";
Decl *FromTU = getTuDecl(Code, Lang_CXX);
ClassTemplateDecl *From = FirstDeclMatcher<ClassTemplateDecl>().match(FromTU, classTemplateDecl());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use auto here and bellow to avoid repeating type name.

@@ -2023,6 +2023,14 @@ Decl *ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
Found = Tag->getDecl();
}

if (D->getDescribedTemplate()) {
if (ClassTemplateDecl *Template = dyn_cast<ClassTemplateDecl>(Found)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use braces for this if and it's else branch.

}
)";
Decl *FromTU = getTuDecl(Code, Lang_CXX);
NamespaceDecl *FromNs = FirstDeclMatcher<NamespaceDecl>().match(FromTU, namespaceDecl());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use auto * where type name is repeated.

Fix for assertion
"Try to import an already imported Decl"
that occurs through calling
ASTNodeImporter::VisitClassTemplateDecl.

The problem is related to import of a CXXRecordDecl that is part
of a template ("template describing record"). If the describing
record is imported (on its own, not as part of the described
template) and this is already existing it is not found and
imported redundantly. When the described template is imported
(already during import of the describing record) the existing
template is found and the assertion is triggered.
@balazske balazske force-pushed the ClassTemplateDecl_AlreadyImported_fix branch from 770d119 to ee3e2c4 Compare March 2, 2018 10:22
@balazske
Copy link
Collaborator Author

balazske commented Mar 2, 2018

Code was reformatted.

Copy link

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG!

@Xazax-hun Xazax-hun merged commit 242968f into Ericsson:ctu-clang5 Mar 2, 2018
@@ -814,6 +814,60 @@ TEST(ImportDecl, ImportFunctionTemplateDecl) {
has(returnStmt(has(integerLiteral(equals(2)))))))))))))))))));
}

TEST(ImportDecl, DISABLED_ImportTemplateDefaultArgument) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nobody (even I) has noticed that this test case does not belong here. This is a test for #310 .

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry, this is TDD, first we have the test, then in an other PR we make it pass! :)

@baloghadamsoftware
Copy link

11 tests fail after merging this.

@balazske
Copy link
Collaborator Author

balazske commented Mar 8, 2018

After merge of #314 (that contains this change too) I did not have failed tests (make check-clang).

@balazske
Copy link
Collaborator Author

Phabricator review:
https://reviews.llvm.org/D47313

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants