ASTImporter: improve support for C++ templates

[a.sidorin]

Summary

• Support template partial specialization
• Avoid infinite recursion in IsStructurallyEquivalent for TemplateArgument with implementing IsStructurallyEquivalent for TemplateName

[khazem]

Thanks very much for this patch! It certainly fixes the infinite recursion issue on our codebase. It LGTM, but I'd like to add a test case before landing it.

[spyffe]

This looks amazing. I have a few minor quibbles and a testing concern, but overall this looks like a great step forward! Thank you!

lib/AST/ASTImporter.cpp

458 ↗

(On Diff #78212)

Is this really an appropriate default result? I would argue for false here so that an error would propagate, as is typical in ASTImporter.
Note that this does disagree with the original source's true but I think that was because we knew we didn't handle anything, whereas now the assumption is we handle everything.

496 ↗

(On Diff #78212)

We should probably also check whether DN1->isIdentifier() == DN2->isIdentifier().

520 ↗

(On Diff #78212)

As above, I'd argue for false here now that we're flipping to the assumption that this code is complete.

4911 ↗

(On Diff #78212)

Could you assign this to a variable here, to avoid the redundant call a line below?

4931 ↗

(On Diff #78212)

This blank line is probably not needed.

7035 ↗

(On Diff #78212)

Is this function properly covered by the test? I would like to see some deeply-neded name specifiers in the test, with entries for all the cases here.
If I'm missing the part of the test that covers this, please let me know.

[a.sidorin]

Address review comments; fix tests.

[a.sidorin]

Thank you! I'll update this review again when I have a test for NestedNameSpecifierLocs.

lib/AST/ASTImporter.cpp

458 ↗

(On Diff #78212)

Good point. Default false will also fail import if a new non-handled property or option will be added in AST and clearly indicate an error so I will change it.

[a.sidorin]

Add a simple test for import of complex NestedNameSpecifierLocs.

[spyffe]

Yeah, that test looks great! Thanks!

lib/AST/ASTImporter.cpp

496 ↗

(On Diff #78212)

Looking at my comment with fresh post Thanksgiving eyes, that would be totally wrong. The IsStructurallyEquivalent is fine.

test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec2.cpp

67 ↗

(On Diff #79054)

ooh, nice!

[khazem]

Sorry for the late comment, but one of the tests that this introduces is breaking on my end:

/usr/local/google/home/khazem/doc/llvm/tools/clang/test/ASTMerge/class-template-partial-spec.cpp:9:11: error: expected string not found in input
// CHECK: /media/build/smrc-llvm/master/llvm/tools/clang/test/ASTMerge/Inputs/class-template-partial-spec2.cpp:21:32: error: external variable 'X1' declared with incompatible types in different translation units ('TwoOptionTemplate<int, double>' vs. 'TwoOptionTemplate<int, float>')
          ^
<stdin>:1:1: note: scanning from here
/usr/local/google/home/khazem/doc/llvm/tools/clang/test/ASTMerge/Inputs/class-template-partial-spec1.cpp:20:30: error: external variable 'X0' defined in multiple translation units
^
<stdin>:7:15: note: possible intended match here
/usr/local/google/home/khazem/doc/llvm/tools/clang/test/ASTMerge/Inputs/class-template-partial-spec2.cpp:21:32: error: external variable 'X1' declared with incompatible types in different translation units ('TwoOptionTemplate<int, double>' vs. 'TwoOptionTemplate<int, float>')
              ^

Is this expected?

[a.sidorin]

I didn't notice this failure but I'll recheck this. Thank you Kareem!

[a.sidorin]

Kareem, I have re-checked it and I cannot see the failure. But I'm not going to commit it until NY holidays end (and, anyway, I will not commit it if somebody has failing tests). Could you describe your configuration?

[a.sidorin]

I got it. I have hard-coded paths in CHECK-lines so these tests are passed on my machine but not on other. Thank you Kareem!

[a.sidorin]

Main revisions: rL292776, rL292778. Sorry for not mentioning them in Differential Revision.

cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp

 // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.1.ast %S/Inputs/class-template-partial-spec1.cpp
 // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.2.ast %S/Inputs/class-template-partial-spec2.cpp
 // RUN: not %clang_cc1 -std=c++1z -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1 | FileCheck %s
 
 static_assert(sizeof(**SingleSource.member) == sizeof(**SingleDest.member));
 static_assert(sizeof(SecondDoubleSource.member) == sizeof(SecondDoubleDest.member));
 static_assert(NumberSource.val == 42);
 static_assert(sizeof(Z0Source.member) == sizeof(char));
 static_assert(sizeof(Dst::Z0Dst.member) == sizeof(double));
 static_assert(sizeof(One::Child1<double, One::Two::Three::Parent<double>>::member) == sizeof(double));
 
-// CHECK: llvm/tools/clang/test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec2.cpp:21:32: error: external variable 'X1' declared with incompatible types in different translation units ('TwoOptionTemplate<int, double>' vs. 'TwoOptionTemplate<int, float>')
-// CHECK: llvm/tools/clang/test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec1.cpp:21:31: note: declared here with type 'TwoOptionTemplate<int, float>'
+// CHECK: Inputs/class-template-partial-spec2.cpp:21:32: error: external variable 'X1' declared with incompatible types in different translation units ('TwoOptionTemplate<int, double>' vs. 'TwoOptionTemplate<int, float>')
+// CHECK: Inputs/class-template-partial-spec1.cpp:21:31: note: declared here with type 'TwoOptionTemplate<int, float>'
 
-// CHECK: llvm/tools/clang/test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec2.cpp:24:29: error: external variable 'X4' declared with incompatible types in different translation units ('TwoOptionTemplate<int, int>' vs. 'TwoOptionTemplate<float, float>')
-// CHECK: llvm/tools/clang/test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec1.cpp:24:33: note: declared here with type 'TwoOptionTemplate<float, float>'
+// CHECK: Inputs/class-template-partial-spec2.cpp:24:29: error: external variable 'X4' declared with incompatible types in different translation units ('TwoOptionTemplate<int, int>' vs. 'TwoOptionTemplate<float, float>')
+// CHECK: Inputs/class-template-partial-spec1.cpp:24:33: note: declared here with type 'TwoOptionTemplate<float, float>'
 
-// CHECK: llvm/tools/clang/test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec1.cpp:38:8: warning: type 'IntTemplateSpec<5, void *>' has incompatible definitions in different translation units
-// CHECK: llvm/tools/clang/test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec1.cpp:39:7: note: field 'member' has type 'int' here
-// CHECK: llvm/tools/clang/test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec2.cpp:39:10: note: field 'member' has type 'double' here
+// CHECK: Inputs/class-template-partial-spec1.cpp:38:8: warning: type 'IntTemplateSpec<5, void *>' has incompatible definitions in different translation units
+// CHECK: Inputs/class-template-partial-spec1.cpp:39:7: note: field 'member' has type 'int' here
+// CHECK: Inputs/class-template-partial-spec2.cpp:39:10: note: field 'member' has type 'double' here
 
-// CHECK: llvm/tools/clang/test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec2.cpp:52:25: error: external variable 'Y3' declared with incompatible types in different translation units ('IntTemplateSpec<2, int>' vs. 'IntTemplateSpec<3, int>')
-// CHECK: llvm/tools/clang/test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec1.cpp:52:25: note: declared here with type 'IntTemplateSpec<3, int>'
+// CHECK: Inputs/class-template-partial-spec2.cpp:52:25: error: external variable 'Y3' declared with incompatible types in different translation units ('IntTemplateSpec<2, int>' vs. 'IntTemplateSpec<3, int>')
+// CHECK: Inputs/class-template-partial-spec1.cpp:52:25: note: declared here with type 'IntTemplateSpec<3, int>'
 
 // CHECK-NOT: static_assert