balazske added a comment.
The summary tells nothing about what is the real problem to fix here. In the
lit test I see that an error message is displayed, this causes the test
failure. The problem is that the structural equivalence check is not good for
this special case. The imported AST contains a class template specialization
for `A`, in this specialization the friend class template `A` has a previous
decl that points to the original friend class template that is in a
`ClassTemplateDecl`. In the specialization the "depth" of template arguments is
0, but in the original template it is 1 (the "to" code at import contains only
the "original template", no specialization). This difference in the "depth"
causes the type mismatch when the specialization is imported.
AST dump of this code can show the problem:
template<class T, T U>
class A;
template<class T, T U>
class A {
public:
template<class P, P Q>
friend class A;
A(T x):x(x){}
private:
T x;
};
A<int,3> a1(0);
It is really interesting that at friend templates the depth is 1 but friend
declarations point to objects outside the class, so really the depth should not
increase in a friend template from this point of view. But this is an AST issue.
================
Comment at: clang/lib/AST/ASTImporter.cpp:5829
+ !IsStructuralMatch(D, FoundTemplate, false))
+ continue;
if (IsStructuralMatch(D, FoundTemplate)) {
----------------
It is not good to use `ParentMap` in the AST importer because it does AST
traversal, even worse if this is done on the To context where the AST is
modified and may be in incomplete state.
This way of fix is probably not good for a case when there is a real structural
in-equivalence, this would be not detected. And the current solution skips this
`FoundDecl` but (at least in the used test code) this should be found, not
skipped. (But we can create code where the skip is correct, if there is a real
structural in-equivalence.)
================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:4218
+ R"(
+ namespace __1{
+
----------------
I think the `namespace __1` is not important for reproduction of this problem.
================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:4234
+ int j=1/i;
+ (void)j;
+ }
----------------
Functions `foo`, `bar`, `main` are not required. It is only important to have a
variable of type `A` like `A<int, 3> a1(0);` in the imported code at getTuDecl.
================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:4247
+ }
+ )",
+ Lang_CXX11);
----------------
The coding format should be aligned to the format of other test codes in this
file, and this is normally same as the clang format guidelines (automatic
reformatting does not work in the test code).
================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:4284
+ Lang_CXX11, "input1.cc");
+ auto *Definition = FirstDeclMatcher<ClassTemplateDecl>().match(
+ FromTU, classTemplateDecl(hasName("A")));
----------------
`Definition` is misleading because this is not the definition, it matches the
first declaration of `A` in the AST. Better name is like `FromA` like in the
other tests, or FromXxx.
================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:4286
+ FromTU, classTemplateDecl(hasName("A")));
+ auto *Template = Import(Definition, Lang_CXX11);
+ EXPECT_TRUE(Template);
----------------
The imported name can be `ToA` or ToXxx or ImportedXxx, this makes real
distinction between the from and to objects.
================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:4288
+ EXPECT_TRUE(Template);
+ auto *TemplateClass = cast<ClassTemplateDecl>(Template);
+ EXPECT_EQ(Fwd->getTemplatedDecl()->getTypeForDecl(),
----------------
This cast is not needed, type of `Template` is already `ClassTemplateDecl*`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156693/new/
https://reviews.llvm.org/D156693
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
