From 40c7806451f484436aa0bdfa53254cf41d3427fa Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Sat, 21 Feb 2015 02:31:57 +0000 Subject: [PATCH] Revert r167816 and replace it with a proper fix for the issue: do not invalidate lookup_iterators and lookup_results for some name within a DeclContext if the lookup results for a *different* name change. llvm-svn: 230121 --- clang/include/clang/AST/DeclBase.h | 91 +++++++++++++++---- .../include/clang/AST/DeclContextInternals.h | 10 +- clang/lib/AST/DeclBase.cpp | 30 +++--- clang/lib/AST/DeclCXX.cpp | 5 +- clang/lib/Sema/SemaInit.cpp | 21 +---- clang/lib/Serialization/ASTWriter.cpp | 6 +- 6 files changed, 104 insertions(+), 59 deletions(-) diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h index 93f6a6fa0f88..ad3f0f88127a 100644 --- a/clang/include/clang/AST/DeclBase.h +++ b/clang/include/clang/AST/DeclBase.h @@ -18,6 +18,7 @@ #include "clang/AST/DeclarationName.h" #include "clang/Basic/Specifiers.h" #include "llvm/ADT/PointerUnion.h" +#include "llvm/ADT/iterator.h" #include "llvm/ADT/iterator_range.h" #include "llvm/Support/Compiler.h" #include "llvm/Support/PrettyStackTrace.h" @@ -1005,9 +1006,65 @@ public: void print(raw_ostream &OS) const override; }; -typedef MutableArrayRef DeclContextLookupResult; +/// \brief The results of name lookup within a DeclContext. This is either a +/// single result (with no stable storage) or a collection of results (with +/// stable storage provided by the lookup table). +class DeclContextLookupResult { + typedef ArrayRef ResultTy; + ResultTy Result; + // If there is only one lookup result, it would be invalidated by + // reallocations of the name table, so store it separately. + NamedDecl *Single; -typedef ArrayRef DeclContextLookupConstResult; + static NamedDecl *const SingleElementDummyList; + +public: + DeclContextLookupResult() : Result(), Single() {} + DeclContextLookupResult(ArrayRef Result) + : Result(Result), Single() {} + DeclContextLookupResult(NamedDecl *Single) + : Result(SingleElementDummyList), Single(Single) {} + + class iterator; + typedef llvm::iterator_adaptor_base IteratorBase; + class iterator : public IteratorBase { + value_type SingleElement; + + public: + iterator() : IteratorBase(), SingleElement() {} + explicit iterator(pointer Pos, value_type Single = nullptr) + : IteratorBase(Pos), SingleElement(Single) {} + + reference operator*() const { + return SingleElement ? SingleElement : IteratorBase::operator*(); + } + }; + typedef iterator const_iterator; + typedef iterator::pointer pointer; + typedef iterator::reference reference; + + iterator begin() const { return iterator(Result.begin(), Single); } + iterator end() const { return iterator(Result.end(), Single); } + + bool empty() const { return Result.empty(); } + pointer data() const { return Single ? &Single : Result.data(); } + size_t size() const { return Single ? 1 : Result.size(); } + reference front() const { return Single ? Single : Result.front(); } + reference back() const { return Single ? Single : Result.back(); } + reference operator[](size_t N) const { return Single ? Single : Result[N]; } + + // FIXME: Remove this from the interface + DeclContextLookupResult slice(size_t N) const { + DeclContextLookupResult Sliced = Result.slice(N); + Sliced.Single = Single; + return Sliced; + } +}; + +// FIXME: Remove this. +typedef DeclContextLookupResult DeclContextLookupConstResult; /// DeclContext - This is used only as base class of specific decl types that /// can act as declaration contexts. These decls are (only the top classes @@ -1520,26 +1577,19 @@ public: /// @brief Checks whether a declaration is in this context. bool containsDecl(Decl *D) const; - /// lookup_iterator - An iterator that provides access to the results - /// of looking up a name within this context. - typedef NamedDecl **lookup_iterator; - - /// lookup_const_iterator - An iterator that provides non-mutable - /// access to the results of lookup up a name within this context. - typedef NamedDecl * const * lookup_const_iterator; - typedef DeclContextLookupResult lookup_result; - typedef DeclContextLookupConstResult lookup_const_result; + typedef lookup_result::iterator lookup_iterator; + + // FIXME: Remove these. + typedef lookup_result lookup_const_result; + typedef lookup_iterator lookup_const_iterator; /// lookup - Find the declarations (if any) with the given Name in /// this context. Returns a range of iterators that contains all of /// the declarations with this name, with object, function, member, /// and enumerator names preceding any tag name. Note that this /// routine will not look into parent contexts. - lookup_result lookup(DeclarationName Name); - lookup_const_result lookup(DeclarationName Name) const { - return const_cast(this)->lookup(Name); - } + lookup_result lookup(DeclarationName Name) const; /// \brief Find the declarations with the given name that are visible /// within this context; don't attempt to retrieve anything from an @@ -1593,7 +1643,16 @@ public: all_lookups_iterator noload_lookups_begin() const; all_lookups_iterator noload_lookups_end() const; - typedef llvm::iterator_range udir_range; + struct udir_iterator; + typedef llvm::iterator_adaptor_base udir_iterator_base; + struct udir_iterator : udir_iterator_base { + udir_iterator(lookup_iterator I) : udir_iterator_base(I) {} + UsingDirectiveDecl *operator*() const; + }; + + typedef llvm::iterator_range udir_range; udir_range using_directives() const; diff --git a/clang/include/clang/AST/DeclContextInternals.h b/clang/include/clang/AST/DeclContextInternals.h index e0830d02ae00..ff37758c2551 100644 --- a/clang/include/clang/AST/DeclContextInternals.h +++ b/clang/include/clang/AST/DeclContextInternals.h @@ -142,23 +142,21 @@ public: /// represents. DeclContext::lookup_result getLookupResult() { if (isNull()) - return DeclContext::lookup_result(DeclContext::lookup_iterator(nullptr), - DeclContext::lookup_iterator(nullptr)); + return DeclContext::lookup_result(); // If we have a single NamedDecl, return it. - if (getAsDecl()) { + if (NamedDecl *ND = getAsDecl()) { assert(!isNull() && "Empty list isn't allowed"); // Data is a raw pointer to a NamedDecl*, return it. - void *Ptr = &Data; - return DeclContext::lookup_result((NamedDecl**)Ptr, (NamedDecl**)Ptr+1); + return DeclContext::lookup_result(ND); } assert(getAsVector() && "Must have a vector at this point"); DeclsTy &Vector = *getAsVector(); // Otherwise, we have a range result. - return DeclContext::lookup_result(Vector.begin(), Vector.end()); + return DeclContext::lookup_result(Vector); } /// HandleRedeclaration - If this is a redeclaration of an existing decl, diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp index b4f1c98f712d..30cca6f6b1be 100644 --- a/clang/lib/AST/DeclBase.cpp +++ b/clang/lib/AST/DeclBase.cpp @@ -1297,12 +1297,14 @@ void DeclContext::buildLookupImpl(DeclContext *DCtx, bool Internal) { } } +NamedDecl *const DeclContextLookupResult::SingleElementDummyList = nullptr; + DeclContext::lookup_result -DeclContext::lookup(DeclarationName Name) { +DeclContext::lookup(DeclarationName Name) const { assert(DeclKind != Decl::LinkageSpec && "Should not perform lookups into linkage specs!"); - DeclContext *PrimaryContext = getPrimaryContext(); + const DeclContext *PrimaryContext = getPrimaryContext(); if (PrimaryContext != this) return PrimaryContext->lookup(Name); @@ -1318,7 +1320,8 @@ DeclContext::lookup(DeclarationName Name) { StoredDeclsMap *Map = LookupPtr.getPointer(); if (LookupPtr.getInt()) - Map = buildLookup(); + // FIXME: Make buildLookup const? + Map = const_cast(this)->buildLookup(); if (!Map) Map = CreateStoredDeclsMap(getParentASTContext()); @@ -1338,19 +1341,19 @@ DeclContext::lookup(DeclarationName Name) { } } - return lookup_result(lookup_iterator(nullptr), lookup_iterator(nullptr)); + return lookup_result(); } StoredDeclsMap *Map = LookupPtr.getPointer(); if (LookupPtr.getInt()) - Map = buildLookup(); + Map = const_cast(this)->buildLookup(); if (!Map) - return lookup_result(lookup_iterator(nullptr), lookup_iterator(nullptr)); + return lookup_result(); StoredDeclsMap::iterator I = Map->find(Name); if (I == Map->end()) - return lookup_result(lookup_iterator(nullptr), lookup_iterator(nullptr)); + return lookup_result(); return I->second.getLookupResult(); } @@ -1387,12 +1390,11 @@ DeclContext::noload_lookup(DeclarationName Name) { } if (!Map) - return lookup_result(lookup_iterator(nullptr), lookup_iterator(nullptr)); + return lookup_result(); StoredDeclsMap::iterator I = Map->find(Name); return I != Map->end() ? I->second.getLookupResult() - : lookup_result(lookup_iterator(nullptr), - lookup_iterator(nullptr)); + : lookup_result(); } void DeclContext::localUncachedLookup(DeclarationName Name, @@ -1574,15 +1576,17 @@ void DeclContext::makeDeclVisibleInContextImpl(NamedDecl *D, bool Internal) { DeclNameEntries.AddSubsequentDecl(D); } +UsingDirectiveDecl *DeclContext::udir_iterator::operator*() const { + return cast(*I); +} + /// Returns iterator range [First, Last) of UsingDirectiveDecls stored within /// this context. DeclContext::udir_range DeclContext::using_directives() const { // FIXME: Use something more efficient than normal lookup for using // directives. In C++, using directives are looked up more than anything else. lookup_const_result Result = lookup(UsingDirectiveDecl::getName()); - return udir_range( - reinterpret_cast(Result.begin()), - reinterpret_cast(Result.end())); + return udir_range(Result.begin(), Result.end()); } //===----------------------------------------------------------------------===// diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp index a796e0b622f4..536a1453aaae 100644 --- a/clang/lib/AST/DeclCXX.cpp +++ b/clang/lib/AST/DeclCXX.cpp @@ -1418,9 +1418,8 @@ CXXMethodDecl::getCorrespondingMethodInClass(const CXXRecordDecl *RD, return nullptr; } - lookup_const_result Candidates = RD->lookup(getDeclName()); - for (NamedDecl * const * I = Candidates.begin(); I != Candidates.end(); ++I) { - CXXMethodDecl *MD = dyn_cast(*I); + for (auto *ND : RD->lookup(getDeclName())) { + CXXMethodDecl *MD = dyn_cast(ND); if (!MD) continue; if (recursivelyOverrides(MD, this)) diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp index c0cf8e0a63aa..9eb7700e9eab 100644 --- a/clang/lib/Sema/SemaInit.cpp +++ b/clang/lib/Sema/SemaInit.cpp @@ -3171,15 +3171,13 @@ static OverloadingResult ResolveConstructorOverload(Sema &S, SourceLocation DeclLoc, MultiExprArg Args, OverloadCandidateSet &CandidateSet, - ArrayRef Ctors, + DeclContext::lookup_result Ctors, OverloadCandidateSet::iterator &Best, bool CopyInitializing, bool AllowExplicit, bool OnlyListConstructors, bool IsListInit) { CandidateSet.clear(); - for (ArrayRef::iterator - Con = Ctors.begin(), ConEnd = Ctors.end(); Con != ConEnd; ++Con) { - NamedDecl *D = *Con; + for (NamedDecl *D : Ctors) { DeclAccessPair FoundDecl = DeclAccessPair::make(D, D->getAccess()); bool SuppressUserConversions = false; @@ -3281,11 +3279,7 @@ static void TryConstructorInitialization(Sema &S, // - Otherwise, if T is a class type, constructors are considered. The // applicable constructors are enumerated, and the best one is chosen // through overload resolution. - DeclContext::lookup_result R = S.LookupConstructors(DestRecordDecl); - // The container holding the constructors can under certain conditions - // be changed while iterating (e.g. because of deserialization). - // To be safe we copy the lookup results to a new container. - SmallVector Ctors(R.begin(), R.end()); + DeclContext::lookup_result Ctors = S.LookupConstructors(DestRecordDecl); OverloadingResult Result = OR_No_Viable_Function; OverloadCandidateSet::iterator Best; @@ -3663,14 +3657,7 @@ static OverloadingResult TryRefInitWithConversionFunction(Sema &S, // to see if there is a suitable conversion. CXXRecordDecl *T1RecordDecl = cast(T1RecordType->getDecl()); - DeclContext::lookup_result R = S.LookupConstructors(T1RecordDecl); - // The container holding the constructors can under certain conditions - // be changed while iterating (e.g. because of deserialization). - // To be safe we copy the lookup results to a new container. - SmallVector Ctors(R.begin(), R.end()); - for (SmallVectorImpl::iterator - CI = Ctors.begin(), CE = Ctors.end(); CI != CE; ++CI) { - NamedDecl *D = *CI; + for (NamedDecl *D : S.LookupConstructors(T1RecordDecl)) { DeclAccessPair FoundDecl = DeclAccessPair::make(D, D->getAccess()); // Find the constructor (which may be a template). diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 07d5d29e44b0..5d508967f259 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -3756,16 +3756,14 @@ ASTWriter::GenerateNameLookupTable(const DeclContext *DC, // Add the constructors. if (!ConstructorDecls.empty()) { Generator.insert(ConstructorName, - DeclContext::lookup_result(ConstructorDecls.begin(), - ConstructorDecls.end()), + DeclContext::lookup_result(ConstructorDecls), Trait); } // Add the conversion functions. if (!ConversionDecls.empty()) { Generator.insert(ConversionName, - DeclContext::lookup_result(ConversionDecls.begin(), - ConversionDecls.end()), + DeclContext::lookup_result(ConversionDecls), Trait); }