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

@@ -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<NamedDecl *> 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<NamedDecl *> 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<NamedDecl *> DeclContextLookupConstResult;
+  static NamedDecl *const SingleElementDummyList;
+
+public:
+  DeclContextLookupResult() : Result(), Single() {}
+  DeclContextLookupResult(ArrayRef<NamedDecl *> Result)
+      : Result(Result), Single() {}
+  DeclContextLookupResult(NamedDecl *Single)
+      : Result(SingleElementDummyList), Single(Single) {}
+
+  class iterator;
+  typedef llvm::iterator_adaptor_base<iterator, ResultTy::iterator,
+                                      std::random_access_iterator_tag,
+                                      NamedDecl *const> 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_result lookup(DeclarationName Name) const;
-  lookup_const_result lookup(DeclarationName Name) const {
-    return const_cast<DeclContext*>(this)->lookup(Name);
-  }
 
   /// \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<UsingDirectiveDecl * const *> udir_range;
+  struct udir_iterator;
+  typedef llvm::iterator_adaptor_base<udir_iterator, lookup_iterator,
+                                      std::random_access_iterator_tag,
+                                      UsingDirectiveDecl *> 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_iterator> udir_range;
 
   udir_range using_directives() const;
 

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),
+      return DeclContext::lookup_result();
-                                        DeclContext::lookup_iterator(nullptr));
 
     // 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(ND);
-      return DeclContext::lookup_result((NamedDecl**)Ptr, (NamedDecl**)Ptr+1);
     }
 
     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,

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<DeclContext*>(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<DeclContext*>(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_result();
-                                         lookup_iterator(nullptr));
 }
 
 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<UsingDirectiveDecl>(*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(
+  return udir_range(Result.begin(), Result.end());
-      reinterpret_cast<UsingDirectiveDecl *const *>(Result.begin()),
-      reinterpret_cast<UsingDirectiveDecl *const *>(Result.end()));
 }
 
 //===----------------------------------------------------------------------===//

clang/lib/AST/DeclCXX.cpp

@@ -1418,9 +1418,8 @@ CXXMethodDecl::getCorrespondingMethodInClass(const CXXRecordDecl *RD,
     return nullptr;
   }
 
-  lookup_const_result Candidates = RD->lookup(getDeclName());
+  for (auto *ND : RD->lookup(getDeclName())) {
-  for (NamedDecl * const * I = Candidates.begin(); I != Candidates.end(); ++I) {
+    CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(ND);
-    CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(*I);
     if (!MD)
       continue;
     if (recursivelyOverrides(MD, this))

clang/lib/Sema/SemaInit.cpp

@@ -3171,15 +3171,13 @@ static OverloadingResult
 ResolveConstructorOverload(Sema &S, SourceLocation DeclLoc,
                            MultiExprArg Args,
                            OverloadCandidateSet &CandidateSet,
-                           ArrayRef<NamedDecl *> Ctors,
+                           DeclContext::lookup_result Ctors,
                            OverloadCandidateSet::iterator &Best,
                            bool CopyInitializing, bool AllowExplicit,
                            bool OnlyListConstructors, bool IsListInit) {
   CandidateSet.clear();
 
-  for (ArrayRef<NamedDecl *>::iterator
+  for (NamedDecl *D : Ctors) {
-         Con = Ctors.begin(), ConEnd = Ctors.end(); Con != ConEnd; ++Con) {
-    NamedDecl *D = *Con;
     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);
+  DeclContext::lookup_result Ctors = 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<NamedDecl*, 16> Ctors(R.begin(), R.end());
 
   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<CXXRecordDecl>(T1RecordType->getDecl());
 
-    DeclContext::lookup_result R = S.LookupConstructors(T1RecordDecl);
+    for (NamedDecl *D : 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<NamedDecl*, 16> Ctors(R.begin(), R.end());
-    for (SmallVectorImpl<NamedDecl *>::iterator
-           CI = Ctors.begin(), CE = Ctors.end(); CI != CE; ++CI) {
-      NamedDecl *D = *CI;
       DeclAccessPair FoundDecl = DeclAccessPair::make(D, D->getAccess());
 
       // Find the constructor (which may be a template).

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(),
+                     DeclContext::lookup_result(ConstructorDecls),
-                                                ConstructorDecls.end()),
                      Trait);
   }
 
   // Add the conversion functions.
   if (!ConversionDecls.empty()) {
     Generator.insert(ConversionName,
-                     DeclContext::lookup_result(ConversionDecls.begin(),
+                     DeclContext::lookup_result(ConversionDecls),
-                                                ConversionDecls.end()),
                      Trait);
   }