[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,

The change looks mostly Ok but there are some comments inline.



================
Comment at: include/clang/AST/DeclContextInternals.h:128
+    DeclsTy &Vec = *getAsVector();
+    DeclsTy::iterator I = std::find(Vec.begin(), Vec.end(), D);
+    return I != Vec.end();
----------------
`auto I = llvm::find(Vec, D)`?


================
Comment at: lib/AST/ASTImporter.cpp:2633
+        // struct/union and in case of anonymous structs.  Would be false
+        // becasue there may be several anonymous/unnamed structs in a class.
+        // E.g. these are both valid:
----------------
because (typo)


================
Comment at: lib/AST/ASTImporter.cpp:2644
 
-        PrevDecl = FoundRecord;
-
-        if (RecordDecl *FoundDef = FoundRecord->getDefinition()) {
-          if ((SearchName && !D->isCompleteDefinition() && !IsFriendTemplate)
-              || (D->isCompleteDefinition() &&
-                  D->isAnonymousStructOrUnion()
-                    == FoundDef->isAnonymousStructOrUnion())) {
-            // The record types structurally match, or the "from" translation
-            // unit only had a forward declaration anyway; call it the same
-            // function.
+        if (IsStructuralMatch(D, FoundRecord)) {
+          RecordDecl *FoundDef = FoundRecord->getDefinition();
----------------
IsStructuralMatch check will be repeated if (!SearchName && IsStructuralMatch). Is it expected behaviour?


================
Comment at: lib/AST/ASTImporter.cpp:2667
       ConflictingDecls.push_back(FoundDecl);
-    }
+    } // for
 
----------------
Szelethus wrote:
> Hah. We should do this more often.
Unfortunately, it is a clear sign that we have to simplify the function. It's better to leave a FIXME instead.


================
Comment at: lib/AST/ASTImporter.cpp:2821
 
-  Importer.MapImported(D, D2);
 
----------------
Are these lines removed due to move of MapImported into Create helper?


================
Comment at: lib/AST/ASTImporter.cpp:4991
         if (IsStructuralMatch(D, FoundTemplate)) {
-          if (!IsFriend) {
-            Importer.MapImported(D->getTemplatedDecl(),
-                                 FoundTemplate->getTemplatedDecl());
-            return Importer.MapImported(D, FoundTemplate);
+          ClassTemplateDecl* TemplateWithDef = getDefinition(FoundTemplate);
+          if (D->isThisDeclarationADefinition() && TemplateWithDef) {
----------------
Space after '*'?


================
Comment at: unittests/AST/ASTImporterTest.cpp:3794
+
+TEST_P(ImportFriendClasses, DeclsFromFriendsShouldBeInRedeclChains2) {
+  Decl *From, *To;
----------------
Will `DeclsFromFriendsShouldBeInRedeclChains` without number appear in another patch?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655