[clang] [clang-tools-extra] First run at removing the linked-list for decls. Tests not run, but … (PR #113983)

Erich Keane via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 29 09:09:56 PDT 2024


https://github.com/erichkeane updated https://github.com/llvm/llvm-project/pull/113983

>From 09c1c890394ecaa66b20cd933ba0d85c2141e34f Mon Sep 17 00:00:00 2001
From: erichkeane <ekeane at nvidia.com>
Date: Mon, 28 Oct 2024 17:00:38 -0700
Subject: [PATCH 1/2] First run at removing the linked-list for decls.  Tests
 not run, but it builds

---
 clang/include/clang/AST/Decl.h     |   4 +-
 clang/include/clang/AST/DeclBase.h |  70 +++++++-----
 clang/include/clang/AST/DeclCXX.h  |   2 +-
 clang/lib/AST/Decl.cpp             |  37 ++++--
 clang/lib/AST/DeclBase.cpp         | 173 ++++++++++++++++++-----------
 clang/lib/AST/DeclPrinter.cpp      |   6 +-
 6 files changed, 188 insertions(+), 104 deletions(-)

diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 7ff35d73df5997..641f790c67824f 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -4893,7 +4893,9 @@ class ExportDecl final : public Decl, public DeclContext {
       return RBraceLoc;
     // No braces: get the end location of the (only) declaration in context
     // (if present).
-    return decls_empty() ? getLocation() : decls_begin()->getEndLoc();
+    // TODO: ERICH: is this going to be a problem?  Previous iterator did a
+    // double-dereference, which doesn't seem right.
+    return decls_empty() ? getLocation() : (*decls_begin())->getEndLoc();
   }
 
   SourceRange getSourceRange() const override LLVM_READONLY {
diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index a3447d19909752..3c53658ecd41af 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -240,14 +240,6 @@ class alignas(8) Decl {
     ModulePrivate
   };
 
-protected:
-  /// The next declaration within the same lexical
-  /// DeclContext. These pointers form the linked list that is
-  /// traversed via DeclContext's decls_begin()/decls_end().
-  ///
-  /// The extra three bits are used for the ModuleOwnershipKind.
-  llvm::PointerIntPair<Decl *, 3, ModuleOwnershipKind> NextInContextAndBits;
-
 private:
   friend class DeclContext;
 
@@ -351,6 +343,13 @@ class alignas(8) Decl {
   LLVM_PREFERRED_TYPE(Linkage)
   mutable unsigned CacheValidAndLinkage : 3;
 
+  // TODO: ERICH: Does this have to be protected? The PointerIntPair was, but it
+  // isn't clear that is necessary.
+  /// Kind of ownership the declaration has, for visibility purposes. See
+  /// declaration of `ModuleOwnershipKind`.
+  LLVM_PREFERRED_TYPE(ModuleOwnershipKind)
+  unsigned ModuleOwnership : 3;
+
   /// Allocate memory for a deserialized declaration.
   ///
   /// This routine must be used to allocate memory for any declaration that is
@@ -394,12 +393,12 @@ class alignas(8) Decl {
 
 protected:
   Decl(Kind DK, DeclContext *DC, SourceLocation L)
-      : NextInContextAndBits(nullptr, getModuleOwnershipKindForChildOf(DC)),
-        DeclCtx(DC), Loc(L), DeclKind(DK), InvalidDecl(false), HasAttrs(false),
+      : DeclCtx(DC), Loc(L), DeclKind(DK), InvalidDecl(false), HasAttrs(false),
         Implicit(false), Used(false), Referenced(false),
         TopLevelDeclInObjCContainer(false), Access(AS_none), FromASTFile(0),
         IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
-        CacheValidAndLinkage(llvm::to_underlying(Linkage::Invalid)) {
+        CacheValidAndLinkage(llvm::to_underlying(Linkage::Invalid)),
+        ModuleOwnership(static_cast<unsigned>(getModuleOwnershipKindForChildOf(DC))) {
     if (StatisticsEnabled) add(DK);
   }
 
@@ -449,8 +448,9 @@ class alignas(8) Decl {
   Kind getKind() const { return static_cast<Kind>(DeclKind); }
   const char *getDeclKindName() const;
 
-  Decl *getNextDeclInContext() { return NextInContextAndBits.getPointer(); }
-  const Decl *getNextDeclInContext() const {return NextInContextAndBits.getPointer();}
+  //TODO: ERICH: figure out how necessary these are? Can be implemented 'dumb-ly'
+  //Decl *getNextDeclInContext() { return NextInContextAndBits.getPointer(); }
+  //const Decl *getNextDeclInContext() const {return NextInContextAndBits.getPointer();}
 
   DeclContext *getDeclContext() {
     if (isInSemaDC())
@@ -867,7 +867,7 @@ class alignas(8) Decl {
 
   /// Get the kind of module ownership for this declaration.
   ModuleOwnershipKind getModuleOwnershipKind() const {
-    return NextInContextAndBits.getInt();
+    return static_cast<ModuleOwnershipKind>(ModuleOwnership);
   }
 
   /// Set whether this declaration is hidden from name lookup.
@@ -876,7 +876,7 @@ class alignas(8) Decl {
              MOK != ModuleOwnershipKind::Unowned && !isFromASTFile() &&
              !hasLocalOwningModuleStorage()) &&
            "no storage available for owning module for this declaration");
-    NextInContextAndBits.setInt(MOK);
+    ModuleOwnership = static_cast<unsigned>(MOK);
   }
 
   unsigned getIdentifierNamespace() const {
@@ -2060,19 +2060,21 @@ class DeclContext {
 
   /// FirstDecl - The first declaration stored within this declaration
   /// context.
-  mutable Decl *FirstDecl = nullptr;
+  ///mutable Decl *FirstDecl = nullptr;
 
   /// LastDecl - The last declaration stored within this declaration
   /// context. FIXME: We could probably cache this value somewhere
   /// outside of the DeclContext, to reduce the size of DeclContext by
   /// another pointer.
-  mutable Decl *LastDecl = nullptr;
+  ///mutable Decl *LastDecl = nullptr;
 
   /// Build up a chain of declarations.
   ///
   /// \returns the first/last pair of declarations.
-  static std::pair<Decl *, Decl *>
-  BuildDeclChain(ArrayRef<Decl*> Decls, bool FieldsAlreadyLoaded);
+  // TODO: ERICH: this ends up being just a filter if it doesn't need to update
+  // next decl.
+  //static std::pair<Decl *, Decl *>
+  //BuildDeclChain(ArrayRef<Decl*> Decls, bool FieldsAlreadyLoaded);
 
   DeclContext(Decl::Kind K);
 
@@ -2305,6 +2307,8 @@ class DeclContext {
   /// for non-namespace contexts).
   void collectAllContexts(SmallVectorImpl<DeclContext *> &Contexts);
 
+  // TODO: ERICH: Remove
+/*
   /// decl_iterator - Iterates through the declarations stored
   /// within this context.
   class decl_iterator {
@@ -2345,14 +2349,22 @@ class DeclContext {
       return x.Current != y.Current;
     }
   };
+  */
+
+  // TODO: ERICH: Put this somewhere better? Rename?
+  using DeclCollection = llvm::SmallVector<Decl*>;
+  mutable DeclCollection OurDecls;
 
+  using decl_iterator = DeclCollection::iterator;
   using decl_range = llvm::iterator_range<decl_iterator>;
 
+
   /// decls_begin/decls_end - Iterate over the declarations stored in
   /// this context.
   decl_range decls() const { return decl_range(decls_begin(), decls_end()); }
   decl_iterator decls_begin() const;
-  decl_iterator decls_end() const { return decl_iterator(); }
+  // TODO: ERICH: Do we need to do the 'load' decls thing here too?
+  decl_iterator decls_end() const;// { return OurDecls.end(); }
   bool decls_empty() const;
 
   /// noload_decls_begin/end - Iterate over the declarations stored in this
@@ -2361,14 +2373,15 @@ class DeclContext {
   decl_range noload_decls() const {
     return decl_range(noload_decls_begin(), noload_decls_end());
   }
-  decl_iterator noload_decls_begin() const { return decl_iterator(FirstDecl); }
-  decl_iterator noload_decls_end() const { return decl_iterator(); }
+  decl_iterator noload_decls_begin() const { return OurDecls.begin(); }
+  decl_iterator noload_decls_end() const { return OurDecls.end(); }
 
   /// specific_decl_iterator - Iterates over a subrange of
   /// declarations stored in a DeclContext, providing only those that
   /// are of type SpecificDecl (or a class derived from it). This
   /// iterator is used, for example, to provide iteration over just
   /// the fields within a RecordDecl (with SpecificDecl = FieldDecl).
+  // TODO: ERICH: Could this just be a llvm_filter_range as well?
   template<typename SpecificDecl>
   class specific_decl_iterator {
     /// Current - The current, underlying declaration iterator, which
@@ -2689,11 +2702,16 @@ class DeclContext {
       DeclContextBits.NeedToReconcileExternalVisibleStorage = true;
   }
 
-  /// Determine whether the given declaration is stored in the list of
-  /// declarations lexically within this context.
+  // TODO: ERICH: Used in ASTReader.cpp
+  ///// Determine whether the given declaration is stored in the list of
+  ///// declarations lexically within this context.
   bool isDeclInLexicalTraversal(const Decl *D) const {
-    return D && (D->NextInContextAndBits.getPointer() || D == FirstDecl ||
-                 D == LastDecl);
+    // TODO: ERICH: Not sure exactly what is happening here? Previous impl seems
+    // to be checking that D is either not last in its declaration, or is this
+    // first or last decl.
+    return D && llvm::find(OurDecls, D) != OurDecls.end();
+    //return D && (D->NextInContextAndBits.getPointer() || D == FirstDecl ||
+    //             D == LastDecl);
   }
 
   void setUseQualifiedLookup(bool use = true) const {
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index 2693cc0e95b4b2..e564387f084415 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -2994,7 +2994,7 @@ class LinkageSpecDecl : public Decl, public DeclContext {
       return getRBraceLoc();
     // No braces: get the end location of the (only) declaration in context
     // (if present).
-    return decls_empty() ? getLocation() : decls_begin()->getEndLoc();
+    return decls_empty() ? getLocation() : (*decls_begin())->getEndLoc();
   }
 
   SourceRange getSourceRange() const override LLVM_READONLY {
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 86913763ef9ff5..c3a2779465ad68 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -5093,7 +5093,9 @@ RecordDecl::field_iterator RecordDecl::field_begin() const {
   // FIXME: Come up with a test case that breaks without definition.
   if (RecordDecl *D = getDefinition(); D && D != this)
     return D->field_begin();
-  return field_iterator(decl_iterator(FirstDecl));
+
+  return field_iterator(decls_begin());
+  //return field_iterator(decl_iterator(FirstDecl));
 }
 
 /// completeDefinition - Notes that the definition of this type is now
@@ -5124,8 +5126,19 @@ bool RecordDecl::isMsStruct(const ASTContext &C) const {
 }
 
 void RecordDecl::reorderDecls(const SmallVectorImpl<Decl *> &Decls) {
-  std::tie(FirstDecl, LastDecl) = DeclContext::BuildDeclChain(Decls, false);
-  LastDecl->NextInContextAndBits.setPointer(nullptr);
+  // std::tie(FirstDecl, LastDecl) = DeclContext::BuildDeclChain(Decls, false);
+  //  TODO: ERICH: This function could possibly be used to steal decls, but i
+  //  don't htink it is doing that, I think the purpose of it is to just have
+  //  current decls reordered. This could presumably have leaked/stolen decls,
+  //  but we'll assume nothing gets lost here.  I also wonder if the uses of
+  //  this could be replaced iwth some sort of std::sort here.
+  //  We could also try harder on the assert here to make sure this is the same
+  //  collection, just sorted differently.
+
+  assert(Decls.size() == OurDecls.size() && "Reordering a different set of decls?");
+  OurDecls.clear();
+  OurDecls.insert(OurDecls.begin(), Decls.begin(), Decls.end());
+  //LastDecl->NextInContextAndBits.setPointer(nullptr);
   setIsRandomized(true);
 }
 
@@ -5151,13 +5164,17 @@ void RecordDecl::LoadFieldsFromExternalStorage() const {
   if (Decls.empty())
     return;
 
-  auto [ExternalFirst, ExternalLast] =
-      BuildDeclChain(Decls,
-                     /*FieldsAlreadyLoaded=*/false);
-  ExternalLast->NextInContextAndBits.setPointer(FirstDecl);
-  FirstDecl = ExternalFirst;
-  if (!LastDecl)
-    LastDecl = ExternalLast;
+  // TODO: ERICH: does an insert from the beginning.  Also, the
+  // FieldsAlreadyLoaded removes the filter criteria, so this is just an
+  // 'insert'.
+  OurDecls.insert(OurDecls.begin(), Decls.begin(), Decls.end());
+  //auto [ExternalFirst, ExternalLast] =
+  //    BuildDeclChain(Decls,
+  //                   /*FieldsAlreadyLoaded=*/false);
+  //ExternalLast->NextInContextAndBits.setPointer(FirstDecl);
+  //FirstDecl = ExternalFirst;
+  //if (!LastDecl)
+  //  LastDecl = ExternalLast;
 }
 
 bool RecordDecl::mayInsertExtraPadding(bool EmitRemark) const {
diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp
index 48b91dca1f6d91..cc672915069ccd 100644
--- a/clang/lib/AST/DeclBase.cpp
+++ b/clang/lib/AST/DeclBase.cpp
@@ -1523,26 +1523,26 @@ void DeclContext::collectAllContexts(SmallVectorImpl<DeclContext *> &Contexts) {
     Contexts.push_back(this);
 }
 
-std::pair<Decl *, Decl *>
-DeclContext::BuildDeclChain(ArrayRef<Decl *> Decls,
-                            bool FieldsAlreadyLoaded) {
-  // Build up a chain of declarations via the Decl::NextInContextAndBits field.
-  Decl *FirstNewDecl = nullptr;
-  Decl *PrevDecl = nullptr;
-  for (auto *D : Decls) {
-    if (FieldsAlreadyLoaded && isa<FieldDecl>(D))
-      continue;
-
-    if (PrevDecl)
-      PrevDecl->NextInContextAndBits.setPointer(D);
-    else
-      FirstNewDecl = D;
-
-    PrevDecl = D;
-  }
-
-  return std::make_pair(FirstNewDecl, PrevDecl);
-}
+//std::pair<Decl *, Decl *>
+//DeclContext::BuildDeclChain(ArrayRef<Decl *> Decls,
+//                            bool FieldsAlreadyLoaded) {
+//  // Build up a chain of declarations via the Decl::NextInContextAndBits field.
+//  Decl *FirstNewDecl = nullptr;
+//  Decl *PrevDecl = nullptr;
+//  for (auto *D : Decls) {
+//    if (FieldsAlreadyLoaded && isa<FieldDecl>(D))
+//      continue;
+//
+//    if (PrevDecl)
+//      PrevDecl->NextInContextAndBits.setPointer(D);
+//    else
+//      FirstNewDecl = D;
+//
+//    PrevDecl = D;
+//  }
+//
+//  return std::make_pair(FirstNewDecl, PrevDecl);
+//}
 
 /// 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
@@ -1582,13 +1582,21 @@ DeclContext::LoadLexicalDeclsFromExternalStorage() const {
 
   // Splice the newly-read declarations into the beginning of the list
   // of declarations.
-  Decl *ExternalFirst, *ExternalLast;
-  std::tie(ExternalFirst, ExternalLast) =
-      BuildDeclChain(Decls, FieldsAlreadyLoaded);
-  ExternalLast->NextInContextAndBits.setPointer(FirstDecl);
-  FirstDecl = ExternalFirst;
-  if (!LastDecl)
-    LastDecl = ExternalLast;
+  //Decl *ExternalFirst, *ExternalLast;
+  //std::tie(ExternalFirst, ExternalLast) =
+  //    BuildDeclChain(Decls, FieldsAlreadyLoaded);
+  //ExternalLast->NextInContextAndBits.setPointer(FirstDecl);
+  //FirstDecl = ExternalFirst;
+  //if (!LastDecl)
+  //  LastDecl = ExternalLast;
+
+  // TODO: ERICH: It isn't clear why these decls are going at the beginning,
+  // but we can do that.  Also have to filter out the 'field decl' if fields are
+  // already loaded, so make_filter_range can do that.
+  auto NewDeclRange = llvm::make_filter_range(Decls, [=](Decl *NewD) {
+    return !FieldsAlreadyLoaded || !isa<FieldDecl>(NewD);
+  });
+  OurDecls.insert(OurDecls.begin(), NewDeclRange.begin(), NewDeclRange.end());
   return true;
 }
 
@@ -1626,19 +1634,37 @@ ExternalASTSource::SetExternalVisibleDeclsForName(const DeclContext *DC,
 DeclContext::decl_iterator DeclContext::decls_begin() const {
   if (hasExternalLexicalStorage())
     LoadLexicalDeclsFromExternalStorage();
-  return decl_iterator(FirstDecl);
+  return OurDecls.begin();
+  //return decl_iterator(FirstDecl);
+}
+
+// TODO: ERICH: we need this now, since we don't know the order of the
+// evaluation of args that use decls_begin/decls_end.
+// LoadLexicalDeclsFromExternalStorage sets the hasExternalLexicalStorage while
+// it is working, so only the 1st one will execute.  Previously it was able to
+// do this as just an 'empty' iterator, because all end iterators in the
+// collection were identical, but we obviously cannot do that here.
+DeclContext::decl_iterator DeclContext::decls_end() const {
+  if (hasExternalLexicalStorage())
+    LoadLexicalDeclsFromExternalStorage();
+  return OurDecls.end();
 }
 
 bool DeclContext::decls_empty() const {
   if (hasExternalLexicalStorage())
     LoadLexicalDeclsFromExternalStorage();
 
-  return !FirstDecl;
+  return OurDecls.empty();
+//  return !FirstDecl;
 }
 
 bool DeclContext::containsDecl(Decl *D) const {
-  return (D->getLexicalDeclContext() == this &&
-          (D->NextInContextAndBits.getPointer() || D == LastDecl));
+  return D->getLexicalDeclContext() == this &&
+         llvm::find(OurDecls, D) != OurDecls.end();
+  // TODO: ERICH: Again, this isn't really checking what it thinks it was
+  // checking?  though at least it is ensuring the lexical context is this one.
+  // return (D->getLexicalDeclContext() == this &&
+  //        (D->NextInContextAndBits.getPointer() || D == LastDecl));
 }
 
 bool DeclContext::containsDeclAndLoad(Decl *D) const {
@@ -1689,28 +1715,36 @@ static bool shouldBeHidden(NamedDecl *D) {
 void DeclContext::removeDecl(Decl *D) {
   assert(D->getLexicalDeclContext() == this &&
          "decl being removed from non-lexical context");
-  assert((D->NextInContextAndBits.getPointer() || D == LastDecl) &&
-         "decl is not in decls list");
-
-  // Remove D from the decl chain.  This is O(n) but hopefully rare.
-  if (D == FirstDecl) {
-    if (D == LastDecl)
-      FirstDecl = LastDecl = nullptr;
-    else
-      FirstDecl = D->NextInContextAndBits.getPointer();
-  } else {
-    for (Decl *I = FirstDecl; true; I = I->NextInContextAndBits.getPointer()) {
-      assert(I && "decl not found in linked list");
-      if (I->NextInContextAndBits.getPointer() == D) {
-        I->NextInContextAndBits.setPointer(D->NextInContextAndBits.getPointer());
-        if (D == LastDecl) LastDecl = I;
-        break;
-      }
-    }
-  }
-
-  // Mark that D is no longer in the decl chain.
-  D->NextInContextAndBits.setPointer(nullptr);
+  // TODO: ERICH: Assert here isn't really meaningful anymore?  Same problem as
+  // below, only checks to see if this is the not-last decl in _A_ context, or
+  // is LastDecl of THIS context.
+  // assert((D->NextInContextAndBits.getPointer()
+  // || D == LastDecl) &&
+  //        "decl is not in decls list");
+
+  const auto *Itr = llvm::find(OurDecls, D);
+  assert(Itr != OurDecls.end() && "decl not found in linked list");
+  OurDecls.erase(Itr);
+
+  //// Remove D from the decl chain.  This is O(n) but hopefully rare.{
+  //if (D == FirstDecl) {
+  //  if (D == LastDecl)
+  //    FirstDecl = LastDecl = nullptr;
+  //  else
+  //    FirstDecl = D->NextInContextAndBits.getPointer();
+  //} else {
+  //  for (Decl *I = FirstDecl; true; I = I->NextInContextAndBits.getPointer()) {
+  //    assert(I && "decl not found in linked list");
+  //    if (I->NextInContextAndBits.getPointer() == D) {
+  //      I->NextInContextAndBits.setPointer(D->NextInContextAndBits.getPointer());
+  //      if (D == LastDecl) LastDecl = I;
+  //      break;
+  //    }
+  //  }
+  //}
+
+  //// Mark that D is no longer in the decl chain.
+  //D->NextInContextAndBits.setPointer(nullptr);
 
   // Remove D from the lookup table if necessary.
   if (isa<NamedDecl>(D)) {
@@ -1744,15 +1778,23 @@ void DeclContext::removeDecl(Decl *D) {
 void DeclContext::addHiddenDecl(Decl *D) {
   assert(D->getLexicalDeclContext() == this &&
          "Decl inserted into wrong lexical context");
-  assert(!D->getNextDeclInContext() && D != LastDecl &&
-         "Decl already inserted into a DeclContext");
-
-  if (FirstDecl) {
-    LastDecl->NextInContextAndBits.setPointer(D);
-    LastDecl = D;
-  } else {
-    FirstDecl = LastDecl = D;
-  }
+  // TODO: ERICH: The original version here isn't really possible, though it
+  // wasn't really checking what it thought it was checking (it was checking: is
+  // already in a Decl-Context where it isn't last, AND isn't last in this
+  // decl). Best we can do I think is to see if it is in this decl context.
+  assert(llvm::find(OurDecls, D) == OurDecls.end() &&
+         "decl already inserted into this DeclContext");
+  // TODO: this isn't really possible now,
+  //assert(!D->getNextDeclInContext() && D != LastDecl &&
+  //       "Decl already inserted into a DeclContext");
+
+  OurDecls.push_back(D);
+//  if (FirstDecl) {
+//    LastDecl->NextInContextAndBits.setPointer(D);
+//    LastDecl = D;
+//  } else {
+//    FirstDecl = LastDecl = D;
+//  }
 
   // Notify a C++ record declaration that we've added a member, so it can
   // update its class-specific state.
@@ -1982,7 +2024,12 @@ void DeclContext::localUncachedLookup(DeclarationName Name,
   // matches.
   // FIXME: If we have lazy external declarations, this will not find them!
   // FIXME: Should we CollectAllContexts and walk them all here?
-  for (Decl *D = FirstDecl; D; D = D->getNextDeclInContext()) {
+  //for (Decl *D = FirstDecl; D; D = D->getNextDeclInContext()) {
+  //  if (auto *ND = dyn_cast<NamedDecl>(D))
+  //    if (ND->getDeclName() == Name)
+  //      Results.push_back(ND);
+  //}
+  for (Decl *D : OurDecls) {
     if (auto *ND = dyn_cast<NamedDecl>(D))
       if (ND->getDeclName() == Name)
         Results.push_back(ND);
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
index 0d51fdbc7e1262..fa5ea3d63949c8 100644
--- a/clang/lib/AST/DeclPrinter.cpp
+++ b/clang/lib/AST/DeclPrinter.cpp
@@ -434,7 +434,7 @@ void DeclPrinter::VisitDeclContext(DeclContext *DC, bool Indent) {
       continue;
 
     // Skip over implicit declarations in pretty-printing mode.
-    if (D->isImplicit())
+    if ((*D)->isImplicit())
       continue;
 
     // Don't print implicit specializations, as they are printed when visiting
@@ -480,7 +480,7 @@ void DeclPrinter::VisitDeclContext(DeclContext *DC, bool Indent) {
     if (isa<AccessSpecDecl>(*D)) {
       Indentation -= Policy.Indentation;
       this->Indent();
-      Print(D->getAccess());
+      Print((*D)->getAccess());
       Out << ":\n";
       Indentation += Policy.Indentation;
       continue;
@@ -532,7 +532,7 @@ void DeclPrinter::VisitDeclContext(DeclContext *DC, bool Indent) {
 
     // Declare target attribute is special one, natural spelling for the pragma
     // assumes "ending" construct so print it here.
-    if (D->hasAttr<OMPDeclareTargetDeclAttr>())
+    if ((*D)->hasAttr<OMPDeclareTargetDeclAttr>())
       Out << "#pragma omp end declare target\n";
   }
 

>From a25e3da03acda87a52a9c56a62ab984b5bbf8345 Mon Sep 17 00:00:00 2001
From: erichkeane <ekeane at nvidia.com>
Date: Tue, 29 Oct 2024 09:07:54 -0700
Subject: [PATCH 2/2] More work to make this work, but is broken

---
 .../bugprone/ArgumentCommentCheck.cpp         | 18 +++++++--
 clang/include/clang/AST/Decl.h                |  6 +--
 clang/include/clang/AST/DeclBase.h            | 38 +++++++++++-------
 clang/include/clang/AST/DeclCXX.h             |  8 ++--
 clang/include/clang/AST/DeclObjC.h            | 40 +++++++++----------
 .../TransEmptyStatementsAndDealloc.cpp        |  4 +-
 clang/lib/ARCMigrate/TransProperties.cpp      |  4 +-
 clang/lib/ARCMigrate/Transforms.cpp           |  4 +-
 clang/lib/AST/Decl.cpp                        |  2 +-
 clang/lib/AST/DeclBase.cpp                    | 14 ++++---
 clang/lib/Analysis/UninitializedValues.cpp    |  4 +-
 clang/lib/InstallAPI/Visitor.cpp              |  9 ++++-
 clang/lib/Sema/SemaCodeComplete.cpp           |  4 +-
 clang/lib/Sema/SemaDeclCXX.cpp                |  2 +-
 clang/lib/Sema/SemaInit.cpp                   |  5 ++-
 clang/lib/Tooling/Syntax/BuildTree.cpp        | 11 ++++-
 16 files changed, 107 insertions(+), 66 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
index 8cdd5d0a564675..0712b486c6e2d6 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -187,10 +187,22 @@ static bool looksLikeExpectMethod(const CXXMethodDecl *Expect) {
          Expect->getNameInfo().getName().isIdentifier() &&
          Expect->getName().starts_with("gmock_");
 }
+
+static Decl *getNextDeclInContext(const Decl *D) {
+  // TODO: ERICH: This again uses declcontext, which we'd very much like to
+  // remove as well. So we'll eventually need to find a way to get here. At
+  // least in this case, we are only checking CXXMethodDecls, which probably
+  // need this link anyway, so this is perhaps fine.
+  const DeclContext *DC = D->getDeclContext();
+  auto *Itr = llvm::find(DC->decls(), D);
+  assert(Itr != DC->decls().end() && "Decl not in own decl context?!");
+  ++Itr;
+  return Itr != DC->decls().end() ? *Itr : nullptr;
+}
 static bool areMockAndExpectMethods(const CXXMethodDecl *Mock,
                                     const CXXMethodDecl *Expect) {
   assert(looksLikeExpectMethod(Expect));
-  return Mock != nullptr && Mock->getNextDeclInContext() == Expect &&
+  return Mock != nullptr && getNextDeclInContext(Mock) == Expect &&
          Mock->getNumParams() == Expect->getNumParams() &&
          Mock->getLocation().isMacroID() &&
          Mock->getNameInfo().getName().isIdentifier() &&
@@ -208,7 +220,7 @@ static const CXXMethodDecl *findMockedMethod(const CXXMethodDecl *Method) {
     if (Ctx == nullptr || !Ctx->isRecord())
       return nullptr;
     for (const auto *D : Ctx->decls()) {
-      if (D->getNextDeclInContext() == Method) {
+      if (getNextDeclInContext(D) == Method) {
         const auto *Previous = dyn_cast<CXXMethodDecl>(D);
         return areMockAndExpectMethods(Previous, Method) ? Previous : nullptr;
       }
@@ -216,7 +228,7 @@ static const CXXMethodDecl *findMockedMethod(const CXXMethodDecl *Method) {
     return nullptr;
   }
   if (const auto *Next =
-          dyn_cast_or_null<CXXMethodDecl>(Method->getNextDeclInContext())) {
+          dyn_cast_or_null<CXXMethodDecl>(getNextDeclInContext(Method))) {
     if (looksLikeExpectMethod(Next) && areMockAndExpectMethods(Method, Next))
       return Method;
   }
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 641f790c67824f..3ac0cb967337d6 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -3987,14 +3987,14 @@ class EnumDecl : public TagDecl {
     const EnumDecl *E = getDefinition();
     if (!E)
       E = this;
-    return enumerator_iterator(E->decls_begin());
+    return enumerator_iterator(E->decls_begin(), E->decls_end());
   }
 
   enumerator_iterator enumerator_end() const {
     const EnumDecl *E = getDefinition();
     if (!E)
       E = this;
-    return enumerator_iterator(E->decls_end());
+    return enumerator_iterator(E->decls_end(), E->decls_end());
   }
 
   /// Return the integer type that enumerators should promote to.
@@ -4357,7 +4357,7 @@ class RecordDecl : public TagDecl {
   field_iterator field_begin() const;
 
   field_iterator field_end() const {
-    return field_iterator(decl_iterator());
+    return field_iterator(decls_end(), decls_end());
   }
 
   // Whether there are any fields (non-static data members) in this record.
diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index 3c53658ecd41af..596ce678003f30 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -2307,13 +2307,15 @@ class DeclContext {
   /// for non-namespace contexts).
   void collectAllContexts(SmallVectorImpl<DeclContext *> &Contexts);
 
-  // TODO: ERICH: Remove
-/*
+  // TODO: ERICH: Remove?  Looks like folks depend on being able to do prefix
+  // increment on the iterators, and we cannot do that when using
+  // iterator_range, as that makes it an operator++ on an rvalue.
+
   /// decl_iterator - Iterates through the declarations stored
   /// within this context.
   class decl_iterator {
     /// Current - The current declaration.
-    Decl *Current = nullptr;
+    Decl **Current = nullptr;
 
   public:
     using value_type = Decl *;
@@ -2323,15 +2325,16 @@ class DeclContext {
     using difference_type = std::ptrdiff_t;
 
     decl_iterator() = default;
-    explicit decl_iterator(Decl *C) : Current(C) {}
+    explicit decl_iterator(Decl **C) : Current(C) {}
 
-    reference operator*() const { return Current; }
+    reference operator*() const { return *Current; }
 
+    // TODO: ERICH: this doesn't make sense anymore?
     // This doesn't meet the iterator requirements, but it's convenient
-    value_type operator->() const { return Current; }
+    value_type operator->() const { return *Current; }
 
     decl_iterator& operator++() {
-      Current = Current->getNextDeclInContext();
+      ++Current;
       return *this;
     }
 
@@ -2349,13 +2352,13 @@ class DeclContext {
       return x.Current != y.Current;
     }
   };
-  */
+
 
   // TODO: ERICH: Put this somewhere better? Rename?
   using DeclCollection = llvm::SmallVector<Decl*>;
   mutable DeclCollection OurDecls;
 
-  using decl_iterator = DeclCollection::iterator;
+//  using decl_iterator = DeclCollection::iterator;
   using decl_range = llvm::iterator_range<decl_iterator>;
 
 
@@ -2373,8 +2376,8 @@ class DeclContext {
   decl_range noload_decls() const {
     return decl_range(noload_decls_begin(), noload_decls_end());
   }
-  decl_iterator noload_decls_begin() const { return OurDecls.begin(); }
-  decl_iterator noload_decls_end() const { return OurDecls.end(); }
+  decl_iterator noload_decls_begin() const { return decl_iterator(OurDecls.begin()); }
+  decl_iterator noload_decls_end() const { return decl_iterator(OurDecls.end()); }
 
   /// specific_decl_iterator - Iterates over a subrange of
   /// declarations stored in a DeclContext, providing only those that
@@ -2388,12 +2391,15 @@ class DeclContext {
     /// will either be NULL or will point to a declaration of
     /// type SpecificDecl.
     DeclContext::decl_iterator Current;
+    // TODO: ERICH: This really likely needs to change.  Having this iterator
+    // have to keep around the extra pointer is unfortunate. Perhaps we can change all uses to use llvm_filtered_range?
+    DeclContext::decl_iterator End;
 
     /// SkipToNextDecl - Advances the current position up to the next
     /// declaration of type SpecificDecl that also meets the criteria
     /// required by Acceptable.
     void SkipToNextDecl() {
-      while (*Current && !isa<SpecificDecl>(*Current))
+      while (Current != End && !isa<SpecificDecl>(*Current))
         ++Current;
     }
 
@@ -2417,7 +2423,7 @@ class DeclContext {
     /// of iterators. For example, if you want Objective-C instance
     /// methods, SpecificDecl will be ObjCMethodDecl and A will be
     /// &ObjCMethodDecl::isInstanceMethod.
-    explicit specific_decl_iterator(DeclContext::decl_iterator C) : Current(C) {
+    explicit specific_decl_iterator(DeclContext::decl_iterator C, DeclContext::decl_iterator E) : Current(C), End(E) {
       SkipToNextDecl();
     }
 
@@ -2464,12 +2470,14 @@ class DeclContext {
     /// will either be NULL or will point to a declaration of
     /// type SpecificDecl.
     DeclContext::decl_iterator Current;
+    // TODO: ERICH: WHY is this not ok being a filtered-range?
+    DeclContext::decl_iterator End;
 
     /// SkipToNextDecl - Advances the current position up to the next
     /// declaration of type SpecificDecl that also meets the criteria
     /// required by Acceptable.
     void SkipToNextDecl() {
-      while (*Current &&
+      while (Current != End &&
              (!isa<SpecificDecl>(*Current) ||
               (Acceptable && !(cast<SpecificDecl>(*Current)->*Acceptable)())))
         ++Current;
@@ -2495,7 +2503,7 @@ class DeclContext {
     /// of iterators. For example, if you want Objective-C instance
     /// methods, SpecificDecl will be ObjCMethodDecl and A will be
     /// &ObjCMethodDecl::isInstanceMethod.
-    explicit filtered_decl_iterator(DeclContext::decl_iterator C) : Current(C) {
+    explicit filtered_decl_iterator(DeclContext::decl_iterator C,DeclContext::decl_iterator E ) : Current(C), End(E) {
       SkipToNextDecl();
     }
 
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index e564387f084415..06c52ecb3b2160 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -666,12 +666,12 @@ class CXXRecordDecl : public RecordDecl {
   /// Method begin iterator.  Iterates in the order the methods
   /// were declared.
   method_iterator method_begin() const {
-    return method_iterator(decls_begin());
+    return method_iterator(decls_begin(), decls_end());
   }
 
   /// Method past-the-end iterator.
   method_iterator method_end() const {
-    return method_iterator(decls_end());
+    return method_iterator(decls_end(), decls_end());
   }
 
   /// Iterator access to constructor members.
@@ -682,11 +682,11 @@ class CXXRecordDecl : public RecordDecl {
   ctor_range ctors() const { return ctor_range(ctor_begin(), ctor_end()); }
 
   ctor_iterator ctor_begin() const {
-    return ctor_iterator(decls_begin());
+    return ctor_iterator(decls_begin(), decls_end());
   }
 
   ctor_iterator ctor_end() const {
-    return ctor_iterator(decls_end());
+    return ctor_iterator(decls_end(), decls_end());
   }
 
   /// An iterator over friend declarations.  All of these are defined
diff --git a/clang/include/clang/AST/DeclObjC.h b/clang/include/clang/AST/DeclObjC.h
index 4663603f797545..d2ff8fca86f150 100644
--- a/clang/include/clang/AST/DeclObjC.h
+++ b/clang/include/clang/AST/DeclObjC.h
@@ -966,11 +966,11 @@ class ObjCContainerDecl : public NamedDecl, public DeclContext {
   prop_range properties() const { return prop_range(prop_begin(), prop_end()); }
 
   prop_iterator prop_begin() const {
-    return prop_iterator(decls_begin());
+    return prop_iterator(decls_begin(), decls_end());
   }
 
   prop_iterator prop_end() const {
-    return prop_iterator(decls_end());
+    return prop_iterator(decls_end(), decls_end());
   }
 
   using instprop_iterator =
@@ -983,11 +983,11 @@ class ObjCContainerDecl : public NamedDecl, public DeclContext {
   }
 
   instprop_iterator instprop_begin() const {
-    return instprop_iterator(decls_begin());
+    return instprop_iterator(decls_begin(), decls_end());
   }
 
   instprop_iterator instprop_end() const {
-    return instprop_iterator(decls_end());
+    return instprop_iterator(decls_end(), decls_end());
   }
 
   using classprop_iterator =
@@ -1000,11 +1000,11 @@ class ObjCContainerDecl : public NamedDecl, public DeclContext {
   }
 
   classprop_iterator classprop_begin() const {
-    return classprop_iterator(decls_begin());
+    return classprop_iterator(decls_begin(), decls_end());
   }
 
   classprop_iterator classprop_end() const {
-    return classprop_iterator(decls_end());
+    return classprop_iterator(decls_end(), decls_end());
   }
 
   // Iterator access to instance/class methods.
@@ -1017,11 +1017,11 @@ class ObjCContainerDecl : public NamedDecl, public DeclContext {
   }
 
   method_iterator meth_begin() const {
-    return method_iterator(decls_begin());
+    return method_iterator(decls_begin(), decls_end());
   }
 
   method_iterator meth_end() const {
-    return method_iterator(decls_end());
+    return method_iterator(decls_end(), decls_end());
   }
 
   using instmeth_iterator =
@@ -1034,11 +1034,11 @@ class ObjCContainerDecl : public NamedDecl, public DeclContext {
   }
 
   instmeth_iterator instmeth_begin() const {
-    return instmeth_iterator(decls_begin());
+    return instmeth_iterator(decls_begin(), decls_end());
   }
 
   instmeth_iterator instmeth_end() const {
-    return instmeth_iterator(decls_end());
+    return instmeth_iterator(decls_end(), decls_end());
   }
 
   using classmeth_iterator =
@@ -1051,11 +1051,11 @@ class ObjCContainerDecl : public NamedDecl, public DeclContext {
   }
 
   classmeth_iterator classmeth_begin() const {
-    return classmeth_iterator(decls_begin());
+    return classmeth_iterator(decls_begin(), decls_end());
   }
 
   classmeth_iterator classmeth_end() const {
-    return classmeth_iterator(decls_end());
+    return classmeth_iterator(decls_end(), decls_end());
   }
 
   // Get the local instance/class method declared in this interface.
@@ -1451,7 +1451,7 @@ class ObjCInterfaceDecl : public ObjCContainerDecl
 
   ivar_iterator ivar_begin() const {
     if (const ObjCInterfaceDecl *Def = getDefinition())
-      return ivar_iterator(Def->decls_begin());
+      return ivar_iterator(Def->decls_begin(), Def->decls_end());
 
     // FIXME: Should make sure no callers ever do this.
     return ivar_iterator();
@@ -1459,7 +1459,7 @@ class ObjCInterfaceDecl : public ObjCContainerDecl
 
   ivar_iterator ivar_end() const {
     if (const ObjCInterfaceDecl *Def = getDefinition())
-      return ivar_iterator(Def->decls_end());
+      return ivar_iterator(Def->decls_end(), Def->decls_end());
 
     // FIXME: Should make sure no callers ever do this.
     return ivar_iterator();
@@ -2441,11 +2441,11 @@ class ObjCCategoryDecl : public ObjCContainerDecl {
   ivar_range ivars() const { return ivar_range(ivar_begin(), ivar_end()); }
 
   ivar_iterator ivar_begin() const {
-    return ivar_iterator(decls_begin());
+    return ivar_iterator(decls_begin(), decls_end());
   }
 
   ivar_iterator ivar_end() const {
-    return ivar_iterator(decls_end());
+    return ivar_iterator(decls_end(), decls_end());
   }
 
   unsigned ivar_size() const {
@@ -2514,11 +2514,11 @@ class ObjCImplDecl : public ObjCContainerDecl {
   }
 
   propimpl_iterator propimpl_begin() const {
-    return propimpl_iterator(decls_begin());
+    return propimpl_iterator(decls_begin(), decls_end());
   }
 
   propimpl_iterator propimpl_end() const {
-    return propimpl_iterator(decls_end());
+    return propimpl_iterator(decls_end(), decls_end());
   }
 
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
@@ -2748,11 +2748,11 @@ class ObjCImplementationDecl : public ObjCImplDecl {
   ivar_range ivars() const { return ivar_range(ivar_begin(), ivar_end()); }
 
   ivar_iterator ivar_begin() const {
-    return ivar_iterator(decls_begin());
+    return ivar_iterator(decls_begin(), decls_end());
   }
 
   ivar_iterator ivar_end() const {
-    return ivar_iterator(decls_end());
+    return ivar_iterator(decls_end(), decls_end());
   }
 
   unsigned ivar_size() const {
diff --git a/clang/lib/ARCMigrate/TransEmptyStatementsAndDealloc.cpp b/clang/lib/ARCMigrate/TransEmptyStatementsAndDealloc.cpp
index e9c21b8106d7dd..6be4ae0100f5c1 100644
--- a/clang/lib/ARCMigrate/TransEmptyStatementsAndDealloc.cpp
+++ b/clang/lib/ARCMigrate/TransEmptyStatementsAndDealloc.cpp
@@ -199,8 +199,8 @@ static void cleanupDeallocOrFinalize(MigrationPass &pass) {
 
   typedef DeclContext::specific_decl_iterator<ObjCImplementationDecl>
     impl_iterator;
-  for (impl_iterator I = impl_iterator(DC->decls_begin()),
-                     E = impl_iterator(DC->decls_end()); I != E; ++I) {
+  for (impl_iterator I = impl_iterator(DC->decls_begin(), DC->decls_end()),
+                     E = impl_iterator(DC->decls_end(), DC->decls_end()); I != E; ++I) {
     ObjCMethodDecl *DeallocM = nullptr;
     ObjCMethodDecl *FinalizeM = nullptr;
     for (auto *MD : I->instance_methods()) {
diff --git a/clang/lib/ARCMigrate/TransProperties.cpp b/clang/lib/ARCMigrate/TransProperties.cpp
index 6d1d950821a07c..f08bef08cd6f90 100644
--- a/clang/lib/ARCMigrate/TransProperties.cpp
+++ b/clang/lib/ARCMigrate/TransProperties.cpp
@@ -102,8 +102,8 @@ class PropertiesRewriter {
     typedef DeclContext::specific_decl_iterator<ObjCPropertyImplDecl>
         prop_impl_iterator;
     for (prop_impl_iterator
-           I = prop_impl_iterator(D->decls_begin()),
-           E = prop_impl_iterator(D->decls_end()); I != E; ++I) {
+           I = prop_impl_iterator(D->decls_begin(), D->decls_end()),
+           E = prop_impl_iterator(D->decls_end(), D->decls_end()); I != E; ++I) {
       ObjCPropertyImplDecl *implD = *I;
       if (implD->getPropertyImplementation() != ObjCPropertyImplDecl::Synthesize)
         continue;
diff --git a/clang/lib/ARCMigrate/Transforms.cpp b/clang/lib/ARCMigrate/Transforms.cpp
index fda0e1c932fc0e..3ffd3ccabe6b8f 100644
--- a/clang/lib/ARCMigrate/Transforms.cpp
+++ b/clang/lib/ARCMigrate/Transforms.cpp
@@ -523,8 +523,8 @@ static void GCRewriteFinalize(MigrationPass &pass) {
 
   typedef DeclContext::specific_decl_iterator<ObjCImplementationDecl>
   impl_iterator;
-  for (impl_iterator I = impl_iterator(DC->decls_begin()),
-       E = impl_iterator(DC->decls_end()); I != E; ++I) {
+  for (impl_iterator I = impl_iterator(DC->decls_begin(), DC->decls_end()),
+       E = impl_iterator(DC->decls_end(), DC->decls_end()); I != E; ++I) {
     for (const auto *MD : I->instance_methods()) {
       if (!MD->hasBody())
         continue;
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index c3a2779465ad68..e53df186cb7e00 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -5094,7 +5094,7 @@ RecordDecl::field_iterator RecordDecl::field_begin() const {
   if (RecordDecl *D = getDefinition(); D && D != this)
     return D->field_begin();
 
-  return field_iterator(decls_begin());
+  return field_iterator(decls_begin(), decls_end());
   //return field_iterator(decl_iterator(FirstDecl));
 }
 
diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp
index cc672915069ccd..0114d9e574d402 100644
--- a/clang/lib/AST/DeclBase.cpp
+++ b/clang/lib/AST/DeclBase.cpp
@@ -499,10 +499,14 @@ bool Decl::isFlexibleArrayMemberLike(
     }
   }
 
+  auto Itr = llvm::find(FD->getParent()->fields(), FD->getCanonicalDecl());
+  assert(Itr != FD->getParent()->field_end() && "Field not in fields?!");
+  return ++Itr != FD->getParent()->field_end();
+
   // Test that the field is the last in the structure.
-  RecordDecl::field_iterator FI(
-      DeclContext::decl_iterator(const_cast<FieldDecl *>(FD)));
-  return ++FI == FD->getParent()->field_end();
+  //RecordDecl::field_iterator FI(
+  //    DeclContext::decl_iterator(const_cast<FieldDecl *>(FD)));
+  //return (++llvm::find(FD->getParent()->fields(), FD)) == FD->getParent()->field_end();
 }
 
 TranslationUnitDecl *Decl::getTranslationUnitDecl() {
@@ -1634,7 +1638,7 @@ ExternalASTSource::SetExternalVisibleDeclsForName(const DeclContext *DC,
 DeclContext::decl_iterator DeclContext::decls_begin() const {
   if (hasExternalLexicalStorage())
     LoadLexicalDeclsFromExternalStorage();
-  return OurDecls.begin();
+  return DeclContext::decl_iterator(OurDecls.begin());
   //return decl_iterator(FirstDecl);
 }
 
@@ -1647,7 +1651,7 @@ DeclContext::decl_iterator DeclContext::decls_begin() const {
 DeclContext::decl_iterator DeclContext::decls_end() const {
   if (hasExternalLexicalStorage())
     LoadLexicalDeclsFromExternalStorage();
-  return OurDecls.end();
+  return DeclContext::decl_iterator(OurDecls.end());
 }
 
 bool DeclContext::decls_empty() const {
diff --git a/clang/lib/Analysis/UninitializedValues.cpp b/clang/lib/Analysis/UninitializedValues.cpp
index bf2f7306186507..c823f0787bb11f 100644
--- a/clang/lib/Analysis/UninitializedValues.cpp
+++ b/clang/lib/Analysis/UninitializedValues.cpp
@@ -95,8 +95,8 @@ class DeclToIndex {
 
 void DeclToIndex::computeMap(const DeclContext &dc) {
   unsigned count = 0;
-  DeclContext::specific_decl_iterator<VarDecl> I(dc.decls_begin()),
-                                               E(dc.decls_end());
+  DeclContext::specific_decl_iterator<VarDecl> I(dc.decls_begin(), dc.decls_end()),
+                                               E(dc.decls_end(), dc.decls_end());
   for ( ; I != E; ++I) {
     const VarDecl *vd = *I;
     if (isTrackedVar(vd, &dc))
diff --git a/clang/lib/InstallAPI/Visitor.cpp b/clang/lib/InstallAPI/Visitor.cpp
index a73ea0b0d124c2..8077516db3d047 100644
--- a/clang/lib/InstallAPI/Visitor.cpp
+++ b/clang/lib/InstallAPI/Visitor.cpp
@@ -695,7 +695,14 @@ bool InstallAPIVisitor::VisitCXXRecordDecl(const CXXRecordDecl *D) {
 
   using var_iter = CXXRecordDecl::specific_decl_iterator<VarDecl>;
   using var_range = iterator_range<var_iter>;
-  for (const auto *Var : var_range(D->decls())) {
+  for (const auto *D : /*var_range(D->decls_begin(), D->decls_end())*/
+
+      // TODO: ERICH: is this what we should be doing everywhere?
+      llvm::make_filter_range(D->decls(), llvm::IsaPred<VarDecl>)
+      ) {
+    const auto *Var = cast<VarDecl>(D);
+
+
     // Skip const static member variables.
     // \code
     // struct S {
diff --git a/clang/lib/Sema/SemaCodeComplete.cpp b/clang/lib/Sema/SemaCodeComplete.cpp
index 16a76ff9b5c241..489b994cc06c8e 100644
--- a/clang/lib/Sema/SemaCodeComplete.cpp
+++ b/clang/lib/Sema/SemaCodeComplete.cpp
@@ -6859,8 +6859,8 @@ void SemaCodeCompletion::CodeCompleteNamespaceDecl(Scope *S) {
     // definition of each namespace.
     std::map<NamespaceDecl *, NamespaceDecl *> OrigToLatest;
     for (DeclContext::specific_decl_iterator<NamespaceDecl>
-             NS(Ctx->decls_begin()),
-         NSEnd(Ctx->decls_end());
+             NS(Ctx->decls_begin(), Ctx->decls_end()),
+         NSEnd(Ctx->decls_end(), Ctx->decls_end());
          NS != NSEnd; ++NS)
       OrigToLatest[NS->getFirstDecl()] = *NS;
 
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 1a691c0e1689d6..a9ba38d4d22f4e 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -9916,7 +9916,7 @@ static CXXConstructorDecl *findUserDeclaredCtor(CXXRecordDecl *RD) {
 
   // Look for constructor templates.
   typedef CXXRecordDecl::specific_decl_iterator<FunctionTemplateDecl> tmpl_iter;
-  for (tmpl_iter TI(RD->decls_begin()), TE(RD->decls_end()); TI != TE; ++TI) {
+  for (tmpl_iter TI(RD->decls_begin(), RD->decls_end()), TE(RD->decls_end(), RD->decls_end()); TI != TE; ++TI) {
     if (CXXConstructorDecl *CD =
           dyn_cast<CXXConstructorDecl>(TI->getTemplatedDecl()))
       return CD;
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 573e90aced3eea..d6b1011382d193 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -2920,8 +2920,9 @@ InitListChecker::CheckDesignatedInitializer(const InitializedEntity &Entity,
       ++FieldIndex;
     }
 
-    RecordDecl::field_iterator Field =
-        RecordDecl::field_iterator(DeclContext::decl_iterator(KnownField));
+    RecordDecl::field_iterator Field = llvm::find(RD->fields(), KnownField);
+    // TODO: ERICH: Used to be:
+        //RecordDecl::field_iterator(DeclContext::decl_iterator(KnownField));
 
     // All of the fields of a union are located at the same place in
     // the initializer list.
diff --git a/clang/lib/Tooling/Syntax/BuildTree.cpp b/clang/lib/Tooling/Syntax/BuildTree.cpp
index 3e50d67f4d6ef0..4c47687da617d8 100644
--- a/clang/lib/Tooling/Syntax/BuildTree.cpp
+++ b/clang/lib/Tooling/Syntax/BuildTree.cpp
@@ -484,7 +484,16 @@ class syntax::TreeBuilder {
     assert((isa<DeclaratorDecl, TypedefNameDecl>(D)) &&
            "only DeclaratorDecl and TypedefNameDecl are supported.");
 
-    const Decl *Next = D->getNextDeclInContext();
+    // TODO: ERICH: We'd love to remove this declcontext in the very near
+    // future, so this sort of hackery is very unfortunate.  It would be great
+    // if we could find a way around doing this.  For now, we can use this.
+    // Declarators and Typedefs are both ones that don't necessarily need to
+    // know their decl context.
+    const DeclContext *DC = D->getDeclContext();
+    auto Itr = llvm::find(DC->decls(), D);
+    assert(Itr != DC->decls().end() && "Decl not in its own decl context?");
+    ++Itr;
+    const Decl *Next = Itr != DC->decls().end() ? *Itr : nullptr;
 
     // There's no next sibling, this one is responsible.
     if (Next == nullptr) {



More information about the cfe-commits mailing list