[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

martong marked 8 inline comments as done.
martong added a comment.

Alexei, thanks for the review!



================
Comment at: lib/AST/ASTImporter.cpp:3002
 
+  // Check if we have found an existing definition.  Returns with that
+  // definition if yes, otherwise returns null.
----------------
a_sidorin wrote:
> I like this lambda. To make the code even better, we can move this lambda outside of VisitFunctionDecl because this method is already pretty big.
Ok, I have created a member function in ASTNodeImporter instead of the lambda.


================
Comment at: unittests/AST/ASTImporterTest.cpp:3862
 
+  void CheckPreviousDecl(Decl *To0, Decl *To1) {
+    ASSERT_NE(To0, To1);
----------------
a_sidorin wrote:
> I don't like numbers. Maybe `To0` and `To1` are `LastDecl` and `ImportedDecl`, correspondingly?
Ok, I have renamed To0 to Prev and To1 to Current as they should form a redecl chain this way: Prev->Current.


================
Comment at: unittests/AST/ASTImporterTest.cpp:3881
+    if (auto *From0F = dyn_cast<FunctionDecl>(To0)) {
+      auto *To0F = cast<FunctionDecl>(To0);
+      if (From0F->getTemplatedKind() ==
----------------
a_sidorin wrote:
> To0 and From0F actually have the same value, and To0F is unused.
Good catch. I removed the second variable.


================
Comment at: unittests/AST/ASTImporterTest.cpp:3884
+          FunctionDecl::TK_FunctionTemplateSpecialization) {
+        EXPECT_EQ(To0->getCanonicalDecl(), To1->getCanonicalDecl());
+        // There may be a hidden fwd spec decl before a spec decl.
----------------
a_sidorin wrote:
> If I don't miss something, this assumption does not depend on To0 and To1 kind and can be moved out of the condition, to the function scope.
Yes, thanks for catching this. I have moved this upward right at the beginning of the function.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58668/new/

https://reviews.llvm.org/D58668