r174577 - Fix handling of module imports adding names to a DeclContext after qualified
Richard Smith
richard-llvm at metafoo.co.uk
Wed Feb 6 19:37:08 PST 2013
Author: rsmith
Date: Wed Feb 6 21:37:08 2013
New Revision: 174577
URL: http://llvm.org/viewvc/llvm-project?rev=174577&view=rev
Log:
Fix handling of module imports adding names to a DeclContext after qualified
name lookup has been performed in that context (this probably only happens in
C++).
1) Whenever we add names to a context, set a flag on it, and if we perform
lookup and discover that the context has had a lookup table built but has the
flag set, update all entries in the lookup table with additional names from
the external source.
2) When marking a DeclContext as having external visible decls, mark the
context in which lookup is performed, not the one we are adding. These won't
be the same if we're adding another copy of a pre-existing namespace.
Modified:
cfe/trunk/include/clang/AST/DeclBase.h
cfe/trunk/include/clang/AST/DeclContextInternals.h
cfe/trunk/lib/AST/DeclBase.cpp
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
cfe/trunk/test/Modules/Inputs/namespaces-left.h
cfe/trunk/test/Modules/Inputs/namespaces-right.h
cfe/trunk/test/Modules/namespaces.cpp
Modified: cfe/trunk/include/clang/AST/DeclBase.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclBase.h?rev=174577&r1=174576&r2=174577&view=diff
==============================================================================
--- cfe/trunk/include/clang/AST/DeclBase.h (original)
+++ cfe/trunk/include/clang/AST/DeclBase.h Wed Feb 6 21:37:08 2013
@@ -928,19 +928,26 @@ class DeclContext {
/// \brief Whether this declaration context also has some external
/// storage that contains additional declarations that are lexically
/// part of this context.
- mutable unsigned ExternalLexicalStorage : 1;
+ mutable bool ExternalLexicalStorage : 1;
/// \brief Whether this declaration context also has some external
/// storage that contains additional declarations that are visible
/// in this context.
- mutable unsigned ExternalVisibleStorage : 1;
+ mutable bool ExternalVisibleStorage : 1;
+
+ /// \brief Whether this declaration context has had external visible
+ /// storage added since the last lookup. In this case, \c LookupPtr's
+ /// invariant may not hold and needs to be fixed before we perform
+ /// another lookup.
+ mutable bool NeedToReconcileExternalVisibleStorage : 1;
/// \brief Pointer to the data structure used to lookup declarations
/// within this context (or a DependentStoredDeclsMap if this is a
/// dependent context), and a bool indicating whether we have lazily
/// omitted any declarations from the map. We maintain the invariant
- /// that, if the map contains an entry for a DeclarationName, then it
- /// contains all relevant entries for that name.
+ /// that, if the map contains an entry for a DeclarationName (and we
+ /// haven't lazily omitted anything), then it contains all relevant
+ /// entries for that name.
mutable llvm::PointerIntPair<StoredDeclsMap*, 1, bool> LookupPtr;
protected:
@@ -963,10 +970,11 @@ protected:
static std::pair<Decl *, Decl *>
BuildDeclChain(ArrayRef<Decl*> Decls, bool FieldsAlreadyLoaded);
- DeclContext(Decl::Kind K)
- : DeclKind(K), ExternalLexicalStorage(false),
- ExternalVisibleStorage(false), LookupPtr(0, false), FirstDecl(0),
- LastDecl(0) { }
+ DeclContext(Decl::Kind K)
+ : DeclKind(K), ExternalLexicalStorage(false),
+ ExternalVisibleStorage(false),
+ NeedToReconcileExternalVisibleStorage(false), LookupPtr(0, false),
+ FirstDecl(0), LastDecl(0) {}
public:
~DeclContext();
@@ -1497,6 +1505,8 @@ public:
/// declarations visible in this context.
void setHasExternalVisibleStorage(bool ES = true) {
ExternalVisibleStorage = ES;
+ if (ES)
+ NeedToReconcileExternalVisibleStorage = true;
}
/// \brief Determine whether the given declaration is stored in the list of
@@ -1512,6 +1522,7 @@ public:
LLVM_ATTRIBUTE_USED void dumpDeclContext() const;
private:
+ void reconcileExternalVisibleStorage();
void LoadLexicalDeclsFromExternalStorage() const;
/// @brief Makes a declaration visible within this context, but
Modified: cfe/trunk/include/clang/AST/DeclContextInternals.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclContextInternals.h?rev=174577&r1=174576&r2=174577&view=diff
==============================================================================
--- cfe/trunk/include/clang/AST/DeclContextInternals.h (original)
+++ cfe/trunk/include/clang/AST/DeclContextInternals.h Wed Feb 6 21:37:08 2013
@@ -97,6 +97,22 @@ public:
== Vec.end() && "list still contains decl");
}
+ /// \brief Remove any declarations which were imported from an external
+ /// AST source.
+ void removeExternalDecls() {
+ if (isNull()) {
+ // Nothing to do.
+ } else if (NamedDecl *Singleton = getAsDecl()) {
+ if (Singleton->isFromASTFile())
+ *this = StoredDeclsList();
+ } else {
+ DeclsTy &Vec = *getAsVector();
+ Vec.erase(std::remove_if(Vec.begin(), Vec.end(),
+ std::mem_fun(&Decl::isFromASTFile)),
+ Vec.end());
+ }
+ }
+
/// getLookupResult - Return an array of all the decls that this list
/// represents.
DeclContext::lookup_result getLookupResult() {
@@ -186,7 +202,7 @@ public:
// All other declarations go at the end of the list, but before any
// tag declarations. But we can be clever about tag declarations
// because there can only ever be one in a scope.
- } else if (Vec.back()->hasTagIdentifierNamespace()) {
+ } else if (!Vec.empty() && Vec.back()->hasTagIdentifierNamespace()) {
NamedDecl *TagD = Vec.back();
Vec.back() = D;
Vec.push_back(TagD);
Modified: cfe/trunk/lib/AST/DeclBase.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclBase.cpp?rev=174577&r1=174576&r2=174577&view=diff
==============================================================================
--- cfe/trunk/lib/AST/DeclBase.cpp (original)
+++ cfe/trunk/lib/AST/DeclBase.cpp Wed Feb 6 21:37:08 2013
@@ -913,6 +913,24 @@ DeclContext::BuildDeclChain(ArrayRef<Dec
return std::make_pair(FirstNewDecl, PrevDecl);
}
+/// \brief We have just acquired external visible storage, and we already have
+/// built a lookup map. For every name in the map, pull in the new names from
+/// the external storage.
+void DeclContext::reconcileExternalVisibleStorage() {
+ assert(NeedToReconcileExternalVisibleStorage);
+ if (!LookupPtr.getPointer())
+ return;
+
+ NeedToReconcileExternalVisibleStorage = false;
+
+ StoredDeclsMap &Map = *LookupPtr.getPointer();
+ ExternalASTSource *Source = getParentASTContext().getExternalSource();
+ for (StoredDeclsMap::iterator I = Map.begin(); I != Map.end(); ++I) {
+ I->second.removeExternalDecls();
+ Source->FindExternalVisibleDeclsByName(this, I->first);
+ }
+}
+
/// \brief Load the declarations within this lexical storage from an
/// external source.
void
@@ -963,9 +981,8 @@ ExternalASTSource::SetNoExternalVisibleD
if (!(Map = DC->LookupPtr.getPointer()))
Map = DC->CreateStoredDeclsMap(Context);
- StoredDeclsList &List = (*Map)[Name];
- assert(List.isNull());
- (void) List;
+ // Add an entry to the map for this name, if it's not already present.
+ (*Map)[Name];
return DeclContext::lookup_result();
}
@@ -975,7 +992,6 @@ ExternalASTSource::SetExternalVisibleDec
DeclarationName Name,
ArrayRef<NamedDecl*> Decls) {
ASTContext &Context = DC->getParentASTContext();
-
StoredDeclsMap *Map;
if (!(Map = DC->LookupPtr.getPointer()))
Map = DC->CreateStoredDeclsMap(Context);
@@ -986,6 +1002,7 @@ ExternalASTSource::SetExternalVisibleDec
if (List.isNull())
List.setOnlyValue(*I);
else
+ // FIXME: Need declarationReplaces handling for redeclarations in modules.
List.AddSubsequentDecl(*I);
}
@@ -1145,6 +1162,10 @@ StoredDeclsMap *DeclContext::buildLookup
/// DeclContext, a DeclContext linked to it, or a transparent context
/// nested within it.
void DeclContext::buildLookupImpl(DeclContext *DCtx) {
+ // FIXME: If buildLookup is supposed to return a complete map, we should not
+ // bail out in buildLookup if hasExternalVisibleStorage. If it is not required
+ // to include names from PCH and modules, we should use the noload_ iterators
+ // here.
for (decl_iterator I = DCtx->decls_begin(), E = DCtx->decls_end();
I != E; ++I) {
Decl *D = *I;
@@ -1175,11 +1196,17 @@ DeclContext::lookup(DeclarationName Name
return PrimaryContext->lookup(Name);
if (hasExternalVisibleStorage()) {
- // If a PCH has a result for this name, and we have a local declaration, we
- // will have imported the PCH result when adding the local declaration.
- // FIXME: For modules, we could have had more declarations added by module
- // imoprts since we saw the declaration of the local name.
- if (StoredDeclsMap *Map = LookupPtr.getPointer()) {
+ if (NeedToReconcileExternalVisibleStorage)
+ reconcileExternalVisibleStorage();
+
+ StoredDeclsMap *Map = LookupPtr.getPointer();
+ if (LookupPtr.getInt())
+ Map = buildLookup();
+
+ // If a PCH/module has a result for this name, and we have a local
+ // declaration, we will have imported the PCH/module result when adding the
+ // local declaration or when reconciling the module.
+ if (Map) {
StoredDeclsMap::iterator I = Map->find(Name);
if (I != Map->end())
return I->second.getLookupResult();
@@ -1224,7 +1251,7 @@ void DeclContext::localUncachedLookup(De
}
// If we have a lookup table, check there first. Maybe we'll get lucky.
- if (Name) {
+ if (Name && !LookupPtr.getInt()) {
if (StoredDeclsMap *Map = LookupPtr.getPointer()) {
StoredDeclsMap::iterator Pos = Map->find(Name);
if (Pos != Map->end()) {
Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=174577&r1=174576&r2=174577&view=diff
==============================================================================
--- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Wed Feb 6 21:37:08 2013
@@ -2122,12 +2122,18 @@ Decl *ASTReader::ReadDeclRecord(DeclID I
// If this declaration is also a declaration context, get the
// offsets for its tables of lexical and visible declarations.
if (DeclContext *DC = dyn_cast<DeclContext>(D)) {
+ // FIXME: This should really be
+ // DeclContext *LookupDC = DC->getPrimaryContext();
+ // but that can walk the redeclaration chain, which might not work yet.
+ DeclContext *LookupDC = DC;
+ if (isa<NamespaceDecl>(DC))
+ LookupDC = DC->getPrimaryContext();
std::pair<uint64_t, uint64_t> Offsets = Reader.VisitDeclContext(DC);
if (Offsets.first || Offsets.second) {
if (Offsets.first != 0)
DC->setHasExternalLexicalStorage(true);
if (Offsets.second != 0)
- DC->setHasExternalVisibleStorage(true);
+ LookupDC->setHasExternalVisibleStorage(true);
if (ReadDeclContextStorage(*Loc.F, DeclsCursor, Offsets,
Loc.F->DeclContextInfos[DC]))
return 0;
@@ -2139,7 +2145,7 @@ Decl *ASTReader::ReadDeclRecord(DeclID I
if (I != PendingVisibleUpdates.end()) {
// There are updates. This means the context has external visible
// storage, even if the original stored version didn't.
- DC->setHasExternalVisibleStorage(true);
+ LookupDC->setHasExternalVisibleStorage(true);
DeclContextVisibleUpdates &U = I->second;
for (DeclContextVisibleUpdates::iterator UI = U.begin(), UE = U.end();
UI != UE; ++UI) {
@@ -2150,8 +2156,9 @@ Decl *ASTReader::ReadDeclRecord(DeclID I
PendingVisibleUpdates.erase(I);
}
- if (!DC->hasExternalVisibleStorage() && DC->hasExternalLexicalStorage())
- DC->setMustBuildLookupTable();
+ if (!LookupDC->hasExternalVisibleStorage() &&
+ DC->hasExternalLexicalStorage())
+ LookupDC->setMustBuildLookupTable();
}
assert(Idx == Record.size());
Modified: cfe/trunk/test/Modules/Inputs/namespaces-left.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/namespaces-left.h?rev=174577&r1=174576&r2=174577&view=diff
==============================================================================
--- cfe/trunk/test/Modules/Inputs/namespaces-left.h (original)
+++ cfe/trunk/test/Modules/Inputs/namespaces-left.h Wed Feb 6 21:37:08 2013
@@ -1,5 +1,12 @@
@import namespaces_top;
+float &global(float);
+float &global2(float);
+
+namespace LookupBeforeImport {
+ float &f(float);
+}
+
namespace N1 { }
namespace N1 {
Modified: cfe/trunk/test/Modules/Inputs/namespaces-right.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/namespaces-right.h?rev=174577&r1=174576&r2=174577&view=diff
==============================================================================
--- cfe/trunk/test/Modules/Inputs/namespaces-right.h (original)
+++ cfe/trunk/test/Modules/Inputs/namespaces-right.h Wed Feb 6 21:37:08 2013
@@ -1,5 +1,12 @@
@import namespaces_top;
+double &global(double);
+double &global2(double);
+
+namespace LookupBeforeImport {
+ double &f(double);
+}
+
namespace N2 { }
namespace N2 { }
Modified: cfe/trunk/test/Modules/namespaces.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/namespaces.cpp?rev=174577&r1=174576&r2=174577&view=diff
==============================================================================
--- cfe/trunk/test/Modules/namespaces.cpp (original)
+++ cfe/trunk/test/Modules/namespaces.cpp Wed Feb 6 21:37:08 2013
@@ -1,9 +1,8 @@
// RUN: rm -rf %t
// RUN: %clang_cc1 -x objective-c++ -fmodules -fmodule-cache-path %t -I %S/Inputs %s -verify
-// Importing modules which add declarations to a pre-existing non-imported
-// overload set does not currently work.
-// XFAIL: *
+int &global(int);
+int &global2(int);
namespace N6 {
char &f(char);
@@ -11,6 +10,13 @@ namespace N6 {
namespace N8 { }
+namespace LookupBeforeImport {
+ int &f(int);
+}
+void testEarly() {
+ int &r = LookupBeforeImport::f(1);
+}
+
@import namespaces_left;
@import namespaces_right;
@@ -18,10 +24,18 @@ void test() {
int &ir1 = N1::f(1);
int &ir2 = N2::f(1);
int &ir3 = N3::f(1);
+ int &ir4 = global(1);
+ int &ir5 = ::global2(1);
float &fr1 = N1::f(1.0f);
float &fr2 = N2::f(1.0f);
+ float &fr3 = global(1.0f);
+ float &fr4 = ::global2(1.0f);
+ float &fr5 = LookupBeforeImport::f(1.0f);
double &dr1 = N2::f(1.0);
double &dr2 = N3::f(1.0);
+ double &dr3 = global(1.0);
+ double &dr4 = ::global2(1.0);
+ double &dr5 = LookupBeforeImport::f(1.0);
}
// Test namespaces merged without a common first declaration.
@@ -54,11 +68,10 @@ void testMergedMerged() {
// Test merging when using anonymous namespaces, which does not
// actually perform any merging.
-// other file: expected-note{{passing argument to parameter here}}
void testAnonymousNotMerged() {
N11::consumeFoo(N11::getFoo()); // expected-error{{cannot initialize a parameter of type 'N11::<anonymous>::Foo *' with an rvalue of type 'N11::<anonymous>::Foo *'}}
N12::consumeFoo(N12::getFoo()); // expected-error{{cannot initialize a parameter of type 'N12::<anonymous>::Foo *' with an rvalue of type 'N12::<anonymous>::Foo *'}}
}
-
-// other file: expected-note{{passing argument to parameter here}}
+// namespaces-right.h: expected-note at 60 {{passing argument to parameter here}}
+// namespaces-right.h: expected-note at 67 {{passing argument to parameter here}}
More information about the cfe-commits
mailing list