AST structural equivalence check for qualified name of record.

[martong]

Is this related to that we have more ODR errors in protobuf since #616 ?

[balazske]

If everything is correct there should be no ODR error at all in protobuf after this fix.

[martong]

This is a side note, but I was thinking about that the StructuralEquivalenceContext is not a real structural check. I mean it does compare names, not just the structure. Probably a better name would be FullEquivalenceContext or TotalEquivalenceContext or StructuralAndNameEquivalenceContext.

[martong]

Also, we have this problem now, because we cannot use the usual lookup mechanism with friends. When we use the normal lookup mechanism then there is no need to compare the names at all, because the compared Decls already have the same name; that's guaranteed by the lookup. Alternatively I was thinking about another solution where we could compare the context of the ImportedFriend and the new friend (D).

Something like this:

--- a/lib/AST/ASTImporter.cpp
+++ b/lib/AST/ASTImporter.cpp
@@ -3363,6 +3363,54 @@ getFriendCountAndPosition(FriendDecl *FD) {
   return std::make_tuple(FriendCount, *FriendPosition);
 }
+
+NamedDecl* getNamedDecl(FriendDecl *FrD) {
+  if (NamedDecl *ND = FrD->getFriendDecl()) {
+    return ND;
+  }
+  if (FrD->getFriendType()) {
+    QualType Ty = FrD->getFriendType()->getType();
+    if (isa<ElaboratedType>(Ty))
+      Ty = cast<ElaboratedType>(Ty)->getNamedType();
+    if (!Ty->isDependentType()) {
+      if (const auto *RTy = dyn_cast<RecordType>(Ty))
+        return RTy->getAsCXXRecordDecl();
+      else if (const auto *SpecTy = dyn_cast<TemplateSpecializationType>(Ty))
+        return SpecTy->getAsCXXRecordDecl();
+      else if (const auto TypedefTy = dyn_cast<TypedefType>(Ty)) {
+        return TypedefTy->getDecl();
+      } else {
+        llvm_unreachable("Unhandled type of friend");
+      }
+    }
+  }
+  // DependentType
+  return nullptr;
+}
+
 ExpectedDecl ASTNodeImporter::VisitFriendDecl(FriendDecl *D) {
   // Import the major distinguishing characteristics of a declaration.
   DeclContext *DC, *LexicalDC;
@@ -3378,6 +3426,24 @@ ExpectedDecl ASTNodeImporter::VisitFriendDecl(FriendDecl *D) {

   while (ImportedFriend) {
     bool Match = false;
+
+    NamedDecl *ImportedFriendND = getNamedDecl(ImportedFriend);
+    NamedDecl *FriendND = getNamedDecl(D);
+    if (ImportedFriendND && FriendND &&
+        ImportedFriendND->getQualifiedNameAsString() !=
+            FriendND->getQualifiedNameAsString()) {
+      ImportedFriend = ImportedFriend->getNextFriend();
+      continue;
+    }
+
     if (D->getFriendDecl() && ImportedFriend->getFriendDecl()) {
       Match =
           isStructuralMatch(D->getFriendDecl(), ImportedFriend->getFriendDecl(),

This seems to be more verbose and since our StructuralEquivalenceContext is already not a pure "structural" check I vote for the solution where we compare all NamedDecls' qualified name if they are not parameters.

[balazske]

625 is a new solution for the same problem. The string comparison is not the best to do in struct eq check so the other solution is preferred. Because the change is not related (in the code) to this the new PR was created, this can be closed.