[PATCH] D156693: [clang][ASTImporter]Skip check friend template declaration in VisitClassTemplateDecl

[Balázs Kéri via Phabricator via cfe-commits]

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