[ASTImporter] Load external Decls when getting field index.

[balazske]

Summary

At equality check of fields without name the index of fields is compared.
At determining the index of a field all fields of the parent context
should be loaded from external source to find the field at all.

[a_sidorin]

Hi Balázs,
The approach is OK but I have some minor comments inline.

lib/AST/ASTImporter.cpp

2840 ↗

(On Diff #157257)

llvm_unreachable?

unittests/AST/ASTImporterTest.cpp

2642 ↗

(On Diff #157257)

I think we can make getFieldIndex() a static method of ASTImporter and remove this loop.

[balazske]

• Added common getFieldIndex.

[balazske]

• Added common getFieldIndex.
• Corrected test ImportUnnamedFieldsInCorrectOrder.

cfe/trunk/include/clang/AST/ASTImporter.h

public:
     /// RecordDecl can be found, we can complete it without the need for
     /// importation, eliminating this loop.
     virtual Decl *GetOriginalDecl(Decl *To) { return nullptr; }
 
     /// Determine whether the given types are structurally
     /// equivalent.
     bool IsStructurallyEquivalent(QualType From, QualType To,
                                   bool Complain = true);
+
+    /// Determine the index of a field in its parent record.
+    /// F should be a field (or indirect field) declaration.
+    /// \returns The index of the field in its parent context, starting from 1.
+    /// 0 is returned on error (parent context is non-record).
+    static unsigned getFieldIndex(Decl *F);
+
   };
 
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_ASTIMPORTER_H

cfe/trunk/lib/AST/ASTImporter.cpp

 #include <cassert>
 #include <cstddef>
 #include <memory>
 #include <type_traits>
 #include <utility>
 
 namespace clang {
 
+  unsigned ASTImporter::getFieldIndex(Decl *F) {
+    assert(F && (isa<FieldDecl>(*F) || isa<IndirectFieldDecl>(*F)) &&
+        "Try to get field index for non-field.");
+
+    auto *Owner = dyn_cast<RecordDecl>(F->getDeclContext());
+    if (!Owner)
+      return 0;
+
+    unsigned Index = 1;
+    for (const auto *D : Owner->decls()) {
+      if (D == F)
+        return Index;
+
+      if (isa<FieldDecl>(*D) || isa<IndirectFieldDecl>(*D))
+        ++Index;
+    }
+
+    llvm_unreachable("Field was not found in its parent context.");
+
+    return 0;
+  }
+
   template <class T>
   SmallVector<Decl*, 2>
   getCanonicalForwardRedeclChain(Redeclarable<T>* D) {
     SmallVector<Decl*, 2> Redecls;
     for (auto *R : D->getFirstDecl()->redecls()) {
       if (R != D->getFirstDecl())
         Redecls.push_back(R);
     }

 Decl *ASTNodeImporter::VisitCXXDestructorDecl(CXXDestructorDecl *D) {
   return VisitCXXMethodDecl(D);
 }
 
 Decl *ASTNodeImporter::VisitCXXConversionDecl(CXXConversionDecl *D) {
   return VisitCXXMethodDecl(D);
 }
 
-static unsigned getFieldIndex(Decl *F) {
-  auto *Owner = dyn_cast<RecordDecl>(F->getDeclContext());
-  if (!Owner)
-    return 0;
-
-  unsigned Index = 1;
-  for (const auto *D : Owner->noload_decls()) {
-    if (D == F)
-      return Index;
-
-    if (isa<FieldDecl>(*D) || isa<IndirectFieldDecl>(*D))
-      ++Index;
-  }
-
-  return Index;
-}
-
 Decl *ASTNodeImporter::VisitFieldDecl(FieldDecl *D) {
   // Import the major distinguishing characteristics of a variable.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
   SourceLocation Loc;
   NamedDecl *ToD;
   if (ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
     return nullptr;
   if (ToD)
     return ToD;
 
   // Determine whether we've already imported this field.
   SmallVector<NamedDecl *, 2> FoundDecls;
   DC->getRedeclContext()->localUncachedLookup(Name, FoundDecls);
   for (auto *FoundDecl : FoundDecls) {
     if (auto *FoundField = dyn_cast<FieldDecl>(FoundDecl)) {
       // For anonymous fields, match up by index.
-      if (!Name && getFieldIndex(D) != getFieldIndex(FoundField))
+      if (!Name &&
+          ASTImporter::getFieldIndex(D) !=
+          ASTImporter::getFieldIndex(FoundField))
         continue;
 
       if (Importer.IsStructurallyEquivalent(D->getType(),
                                             FoundField->getType())) {
         Importer.MapImported(D, FoundField);
         return FoundField;
       }
 
if (ToD)
     return ToD;
 
   // Determine whether we've already imported this field.
   SmallVector<NamedDecl *, 2> FoundDecls;
   DC->getRedeclContext()->localUncachedLookup(Name, FoundDecls);
   for (unsigned I = 0, N = FoundDecls.size(); I != N; ++I) {
     if (auto *FoundField = dyn_cast<IndirectFieldDecl>(FoundDecls[I])) {
       // For anonymous indirect fields, match up by index.
-      if (!Name && getFieldIndex(D) != getFieldIndex(FoundField))
+      if (!Name &&
+          ASTImporter::getFieldIndex(D) !=
+          ASTImporter::getFieldIndex(FoundField))
         continue;
 
       if (Importer.IsStructurallyEquivalent(D->getType(),
                                             FoundField->getType(),
                                             !Name.isEmpty())) {
         Importer.MapImported(D, FoundField);
         return FoundField;
       }

cfe/trunk/test/ASTMerge/unnamed_fields/Inputs/il.cpp

+void f(int X, int Y, bool Z) {
+  auto x = [X, Y, Z] { (void)Z; };
+}

cfe/trunk/test/ASTMerge/unnamed_fields/test.cpp

+// RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/il.cpp
+// RUN: %clang_cc1 -ast-merge %t.1.ast -fsyntax-only %s 2>&1 | FileCheck --allow-empty %s
+// CHECK-NOT: warning: field '' declared with incompatible types in different translation units ('bool' vs. 'int')

cfe/trunk/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterTestBase, ImportUnnamedStructsWithRecursingField) {
   auto *R1 = getRecordDecl(Entry1);
   EXPECT_NE(R0, R1);
   EXPECT_TRUE(MatchVerifier<RecordDecl>().match(
       R0, recordDecl(has(fieldDecl(hasName("next"))))));
   EXPECT_TRUE(MatchVerifier<RecordDecl>().match(
       R1, recordDecl(has(fieldDecl(hasName("next"))))));
 }
 
+TEST_P(ASTImporterTestBase, ImportUnnamedFieldsInCorrectOrder) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      void f(int X, int Y, bool Z) {
+        (void)[X, Y, Z] { (void)Z; };
+      }
+      )",
+      Lang_CXX11, "input0.cc");
+  auto *FromF = FirstDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("f")));
+  auto *ToF = cast_or_null<FunctionDecl>(Import(FromF, Lang_CXX11));
+  EXPECT_TRUE(ToF);
+
+  CXXRecordDecl *FromLambda =
+      cast<LambdaExpr>(cast<CStyleCastExpr>(cast<CompoundStmt>(
+          FromF->getBody())->body_front())->getSubExpr())->getLambdaClass();
+
+  auto *ToLambda = cast_or_null<CXXRecordDecl>(Import(FromLambda, Lang_CXX11));
+  EXPECT_TRUE(ToLambda);
+
+  // Check if the fields of the lambda class are imported in correct order.
+  unsigned FromIndex = 0u;
+  for (auto *FromField : FromLambda->fields()) {
+    ASSERT_FALSE(FromField->getDeclName());
+    auto *ToField = cast_or_null<FieldDecl>(Import(FromField, Lang_CXX11));
+    EXPECT_TRUE(ToField);
+    unsigned ToIndex = ASTImporter::getFieldIndex(ToField);
+    EXPECT_EQ(ToIndex, FromIndex + 1);
+    ++FromIndex;
+  }
+
+  EXPECT_EQ(FromIndex, 3u);
+}
+
 struct DeclContextTest : ASTImporterTestBase {};
 
 TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
   Decl *TU = getTuDecl(
       R"(
       namespace NS {
 
       template <typename T>