[modules] When merging class definitions, make the retained definition visible

if the merged definition is visible, and perform lookups into all merged copies
of the definition (not just for special members) so that we can complete the
redecl chains for members of the class.

llvm-svn: 233420

clang/lib/Serialization/ASTReader.cpp

@@ -6730,20 +6730,14 @@ ASTReader::FindExternalVisibleDeclsByName(const DeclContext *DC,
   // individually, because finding an entity in one of them doesn't imply that
   // we can't find a different entity in another one.
   if (isa<CXXRecordDecl>(DC)) {
-    auto Kind = Name.getNameKind();
-    if (Kind == DeclarationName::CXXConstructorName ||
-        Kind == DeclarationName::CXXDestructorName ||
-        (Kind == DeclarationName::CXXOperatorName &&
-         Name.getCXXOverloadedOperator() == OO_Equal)) {
-      auto Merged = MergedLookups.find(DC);
-      if (Merged != MergedLookups.end()) {
-        for (unsigned I = 0; I != Merged->second.size(); ++I) {
-          const DeclContext *Context = Merged->second[I];
-          LookUpInContexts(Context);
-          // We might have just added some more merged lookups. If so, our
-          // iterator is now invalid, so grab a fresh one before continuing.
-          Merged = MergedLookups.find(DC);
-        }
+    auto Merged = MergedLookups.find(DC);
+    if (Merged != MergedLookups.end()) {
+      for (unsigned I = 0; I != Merged->second.size(); ++I) {
+        const DeclContext *Context = Merged->second[I];
+        LookUpInContexts(Context);
+        // We might have just added some more merged lookups. If so, our
+        // iterator is now invalid, so grab a fresh one before continuing.
+        Merged = MergedLookups.find(DC);
       }
     }
   }

clang/lib/Serialization/ASTReaderDecl.cpp

@@ -1385,6 +1385,29 @@ void ASTDeclReader::MergeDefinitionData(
          "merging class definition into non-definition");
   auto &DD = *D->DefinitionData.getNotUpdated();
 
+  // If the new definition has new special members, let the name lookup
+  // code know that it needs to look in the new definition too.
+  //
+  // FIXME: We only need to do this if the merged definition declares members
+  // that this definition did not declare, or if it defines members that this
+  // definition did not define.
+  if (DD.Definition != MergeDD.Definition) {
+    Reader.MergedLookups[DD.Definition].push_back(MergeDD.Definition);
+    DD.Definition->setHasExternalVisibleStorage();
+
+    if (DD.Definition->isHidden()) {
+      // If MergeDD is visible or becomes visible, make the definition visible.
+      if (!MergeDD.Definition->isHidden())
+        DD.Definition->Hidden = false;
+      else {
+        auto SubmoduleID = MergeDD.Definition->getOwningModuleID();
+        assert(SubmoduleID && "hidden definition in no module");
+        Reader.HiddenNamesMap[Reader.getSubmodule(SubmoduleID)]
+              .HiddenDecls.push_back(DD.Definition);
+      }
+    }
+  }
+
   auto PFDI = Reader.PendingFakeDefinitionData.find(&DD);
   if (PFDI != Reader.PendingFakeDefinitionData.end() &&
       PFDI->second == ASTReader::PendingFakeDefinitionKind::Fake) {
@@ -1401,17 +1424,6 @@ void ASTDeclReader::MergeDefinitionData(
     return;
   }
 
-  // If the new definition has new special members, let the name lookup
-  // code know that it needs to look in the new definition too.
-  //
-  // FIXME: We only need to do this if the merged definition declares members
-  // that this definition did not declare, or if it defines members that this
-  // definition did not define.
-  if (MergeDD.DeclaredSpecialMembers && DD.Definition != MergeDD.Definition) {
-    Reader.MergedLookups[DD.Definition].push_back(MergeDD.Definition);
-    DD.Definition->setHasExternalVisibleStorage();
-  }
-
   // FIXME: Move this out into a .def file?
   bool DetectedOdrViolation = false;
 #define OR_FIELD(Field) DD.Field |= MergeDD.Field;

clang/test/Modules/Inputs/redecl-add-after-load-decls.h

@@ -16,9 +16,9 @@ typedef C::A CB;
 constexpr int C_test(bool b) { return b ? C::variable : C::function(); }
 
 struct D {
-  struct A; // expected-note {{forward}}
+  struct A;
   static const int variable;
-  static constexpr int function(); // expected-note {{here}}
+  static constexpr int function();
 };
 typedef D::A DB;
-constexpr int D_test(bool b) { return b ? D::variable : D::function(); } // expected-note {{subexpression}} expected-note {{undefined}}
+constexpr int D_test(bool b) { return b ? D::variable : D::function(); }

clang/test/Modules/Inputs/submodules-merge-defs/module.modulemap

@@ -9,3 +9,8 @@ module "redef" {
   // Do not re-export stuff.use
   use "stuff"
 }
+
+module "merged-defs" {
+  header "merged-defs.h"
+  use "stuff"
+}

clang/test/Modules/cxx-templates.cpp

@@ -125,10 +125,7 @@ void g() {
 static_assert(Outer<int>::Inner<int>::f() == 1, "");
 static_assert(Outer<int>::Inner<int>::g() == 2, "");
 
-// FIXME: We're too lazy in merging class definitions to find the definition
-// of this function.
-static_assert(MergeTemplateDefinitions<int>::f() == 1, ""); // expected-error {{constant expression}} expected-note {{undefined}}
-// expected-note@cxx-templates-c.h:10 {{here}}
+static_assert(MergeTemplateDefinitions<int>::f() == 1, "");
 static_assert(MergeTemplateDefinitions<int>::g() == 2, "");
 
 RedeclaredAsFriend<int> raf1;

clang/test/Modules/redecl-add-after-load.cpp

@@ -2,8 +2,9 @@
 // RUN: %clang_cc1 -x objective-c++ -fmodules -fno-modules-error-recovery -fmodules-cache-path=%t -I %S/Inputs %s -verify -std=c++11
 // RUN: %clang_cc1 -x objective-c++ -fmodules -fno-modules-error-recovery -fmodules-cache-path=%t -I %S/Inputs %s -verify -std=c++11 -DIMPORT_DECLS
 
-#ifdef IMPORT_DECLS
 // expected-no-diagnostics
+
+#ifdef IMPORT_DECLS
 @import redecl_add_after_load_decls;
 #else
 typedef struct A B;
@@ -24,12 +25,12 @@ typedef C::A CB;
 constexpr int C_test(bool b) { return b ? C::variable : C::function(); }
 
 struct D {
-  struct A; // expected-note {{forward}}
+  struct A;
   static const int variable;
-  static constexpr int function(); // expected-note {{here}}
+  static constexpr int function();
 };
 typedef D::A DB;
-constexpr int D_test(bool b) { return b ? D::variable : D::function(); } // expected-note {{undefined}}
+constexpr int D_test(bool b) { return b ? D::variable : D::function(); }
 #endif
 
 @import redecl_add_after_load;
@@ -46,14 +47,6 @@ CB struct_struct_test;
 constexpr int struct_variable_test = C_test(true);
 constexpr int struct_function_test = C_test(false);
 
-// FIXME: We should accept this, but we're currently too lazy when merging class
-// definitions to determine that the definitions in redecl_add_after_load are
-// definitions of these entities.
 DB merged_struct_struct_test;
 constexpr int merged_struct_variable_test = D_test(true);
 constexpr int merged_struct_function_test = D_test(false);
-#ifndef IMPORT_DECLS
-// expected-error@-4 {{incomplete}}
-// @-4: definition of D::variable must be emitted, so it gets imported eagerly
-// expected-error@-4 {{constant}} expected-note@-4 {{in call to}}
-#endif

clang/test/Modules/submodules-merge-defs.cpp

@@ -1,4 +1,5 @@
 // RUN: rm -rf %t
+// RUN: %clang_cc1 -x c++ -std=c++11 -fmodules-cache-path=%t -fmodules -I %S/Inputs/submodules-merge-defs %s -verify -fno-modules-error-recovery -DTEXTUAL
 // RUN: %clang_cc1 -x c++ -std=c++11 -fmodules-cache-path=%t -fmodules -I %S/Inputs/submodules-merge-defs %s -verify -fno-modules-error-recovery
 
 // Trigger import of definitions, but don't make them visible.
@@ -27,7 +28,11 @@ D::X pre_dx; // expected-error +{{must be imported}}
 int pre_use_dx = use_dx(pre_dx);
 
 // Make definitions from second module visible.
+#ifdef TEXTUAL
 #include "import-and-redefine.h"
+#else
+#include "merged-defs.h"
+#endif
 
 A post_a;
 int post_use_a = use_a(post_a);