[PATCH] D46950: [ASTImporter] Fix duplicate class template definitions problem

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hello Gabor,

The patch is fine, I have found only some style issues.



================
Comment at: lib/AST/ASTImporter.cpp:4073
 
+// Returns the definition for a (forwad) declaration of a ClassTemplateDecl, if
+// it has any definition in the redecl chain.
----------------
forward


================
Comment at: lib/AST/ASTImporter.cpp:4121
 
-          // We found a forward declaration but the class to be imported has a
-          // definition.
-          // FIXME Add this forward declaration to the redeclaration chain.
-          if (D->isThisDeclarationADefinition() &&
-              !FoundTemplate->isThisDeclarationADefinition())
+        // The class to be imported has a definition.
+        if (D->isThisDeclarationADefinition()) {
----------------
is a definition?


================
Comment at: lib/AST/ASTImporter.cpp:4124
+          // Lookup will find the fwd decl only if that is more recent than the
+          // definition. So, lets try to get the definition if that is available
+          // in the redecl chain.
----------------
let's; if it is available


================
Comment at: lib/AST/ASTImporter.cpp:4126
+          // in the redecl chain.
+          ClassTemplateDecl* TemplateWithDef = getDefinition(FoundTemplate);
+          if (!TemplateWithDef)
----------------
* is misplaced here.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1550
+    ASTImporterTestBase,
+    ImportDefinitionOfClassTemplateSpecializationIfThereIsAnExistingFwdDeclAndDefinition) {
+  Decl *ToTU = getToTuDecl(
----------------
Although long names make me happy, this one exceeds 80-char limit. We can abbreviate "Specialization" to "Spec".


================
Comment at: unittests/AST/DeclMatcher.h:49
+  using UnaryPredicate = std::function<bool(const NodeType *)>;
+  UnaryPredicate predicate;
+  unsigned count = 0;
----------------
Predicate, Count (member names should start with capital).


Repository:
  rC Clang

https://reviews.llvm.org/D46950