From e308538c71e1cb98b8be82081f83121a2708dd99 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Fri, 27 Mar 2015 21:16:39 +0000 Subject: [PATCH] [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. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@233420 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Serialization/ASTReader.cpp | 22 +++++------- lib/Serialization/ASTReaderDecl.cpp | 34 +++++++++++++------ .../Inputs/redecl-add-after-load-decls.h | 6 ++-- .../submodules-merge-defs/module.modulemap | 5 +++ test/Modules/cxx-templates.cpp | 5 +-- test/Modules/redecl-add-after-load.cpp | 17 +++------- test/Modules/submodules-merge-defs.cpp | 5 +++ 7 files changed, 50 insertions(+), 44 deletions(-) diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp index 4d5141bf92..73c4495861 100644 --- a/lib/Serialization/ASTReader.cpp +++ b/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(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); } } } diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp index de2c625306..5e911b41b7 100644 --- a/lib/Serialization/ASTReaderDecl.cpp +++ b/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; diff --git a/test/Modules/Inputs/redecl-add-after-load-decls.h b/test/Modules/Inputs/redecl-add-after-load-decls.h index fbe6b9387a..a4227971a8 100644 --- a/test/Modules/Inputs/redecl-add-after-load-decls.h +++ b/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(); } diff --git a/test/Modules/Inputs/submodules-merge-defs/module.modulemap b/test/Modules/Inputs/submodules-merge-defs/module.modulemap index 5c7ee8a89a..82abdb088f 100644 --- a/test/Modules/Inputs/submodules-merge-defs/module.modulemap +++ b/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" +} diff --git a/test/Modules/cxx-templates.cpp b/test/Modules/cxx-templates.cpp index 41b0f2cd92..00fc38b522 100644 --- a/test/Modules/cxx-templates.cpp +++ b/test/Modules/cxx-templates.cpp @@ -125,10 +125,7 @@ void g() { static_assert(Outer::Inner::f() == 1, ""); static_assert(Outer::Inner::g() == 2, ""); -// FIXME: We're too lazy in merging class definitions to find the definition -// of this function. -static_assert(MergeTemplateDefinitions::f() == 1, ""); // expected-error {{constant expression}} expected-note {{undefined}} -// expected-note@cxx-templates-c.h:10 {{here}} +static_assert(MergeTemplateDefinitions::f() == 1, ""); static_assert(MergeTemplateDefinitions::g() == 2, ""); RedeclaredAsFriend raf1; diff --git a/test/Modules/redecl-add-after-load.cpp b/test/Modules/redecl-add-after-load.cpp index 53e54c84cc..8ca70ad958 100644 --- a/test/Modules/redecl-add-after-load.cpp +++ b/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 diff --git a/test/Modules/submodules-merge-defs.cpp b/test/Modules/submodules-merge-defs.cpp index 86e50368a9..3bc52fee82 100644 --- a/test/Modules/submodules-merge-defs.cpp +++ b/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); -- 2.40.0