[llvm] 1c15377 - Recommit: CFGDiff: Simplify/common the begin/end implementations to use a common range helper""

[David Blaikie via llvm-commits]

Author: David Blaikie
Date: 2020-03-19T18:21:14-07:00
New Revision: 1c1537749615a9be98a33482cdf6bf6f602f1eed

URL: https://github.com/llvm/llvm-project/commit/1c1537749615a9be98a33482cdf6bf6f602f1eed
DIFF: https://github.com/llvm/llvm-project/commit/1c1537749615a9be98a33482cdf6bf6f602f1eed.diff

LOG: Recommit: CFGDiff: Simplify/common the begin/end implementations to use a common range helper""

(would be nice to revisit the CFG traits and change them to use ranges
rather than begin/end - if anyone wants to do that refactor)

Also use more auto because writing the names of range utilty iterators
isn't helping readability here - they're sort of implementation details
for the most part, especially once you nest a few different filtering
and adapting iterators.

The fix (shooting from the hip since I couldn't reproduce this locally)
was to capture by value in a lambda used in a filtering iterator -
because the iterator would persist beyond the lifetime of the function
(as the iterators are returned to callers).

Originally committed in 79a7ed92a9b135212a6a271dd8dbc625038c8f06.
This was reverted in 4a7f2032a350bc7eefd26709563f65216df3e2ce.

Added: 
    

Modified: 
    llvm/include/llvm/IR/CFGDiff.h

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/IR/CFGDiff.h b/llvm/include/llvm/IR/CFGDiff.h
index f40434cc5d4e..cc38addd058e 100644
--- a/llvm/include/llvm/IR/CFGDiff.h
+++ b/llvm/include/llvm/IR/CFGDiff.h
@@ -158,58 +158,44 @@ template <typename GraphT, bool InverseGraph = false, bool InverseEdge = false,
           typename GT = GraphTraits<GraphT>>
 struct CFGViewChildren {
   using DataRef = const GraphDiff<typename GT::NodeRef, InverseGraph> *;
-  using RawNodeRef = typename GT::NodeRef;
-  using NodeRef = std::pair<DataRef, RawNodeRef>;
-
-  using ExistingChildIterator =
-      WrappedPairNodeDataIterator<typename GT::ChildIteratorType, NodeRef,
-                                  DataRef>;
-  struct DeletedEdgesFilter {
-    RawNodeRef BB;
-    DeletedEdgesFilter(RawNodeRef BB) : BB(BB){};
-    bool operator()(NodeRef N) const {
-      return !N.first->ignoreChild(BB, N.second, InverseEdge);
-    }
-  };
-  using FilterExistingChildrenIterator =
-      filter_iterator<ExistingChildIterator, DeletedEdgesFilter>;
-
-  using vec_iterator = typename SmallVectorImpl<RawNodeRef>::const_iterator;
-  using AddNewChildrenIterator =
-      WrappedPairNodeDataIterator<vec_iterator, NodeRef, DataRef>;
-  using ChildIteratorType =
-      concat_iterator<NodeRef, FilterExistingChildrenIterator,
-                      AddNewChildrenIterator>;
-
-  static ChildIteratorType child_begin(NodeRef N) {
-    auto InsertVec = N.first->getAddedChildren(N.second, InverseEdge);
-    // filter iterator init:
-    auto firstit = make_filter_range(
-        make_range<ExistingChildIterator>({GT::child_begin(N.second), N.first},
-                                          {GT::child_end(N.second), N.first}),
-        DeletedEdgesFilter(N.second));
-    // new inserts iterator init:
-    auto secondit = make_range<AddNewChildrenIterator>(
-        {InsertVec.begin(), N.first}, {InsertVec.end(), N.first});
+  using NodeRef = std::pair<DataRef, typename GT::NodeRef>;
 
-    return concat_iterator<NodeRef, FilterExistingChildrenIterator,
-                           AddNewChildrenIterator>(firstit, secondit);
+  template<typename Range>
+  static auto makeChildRange(Range &&R, DataRef DR) {
+    using Iter = WrappedPairNodeDataIterator<decltype(std::forward<Range>(R).begin()), NodeRef, DataRef>;
+    return make_range(Iter(R.begin(), DR), Iter(R.end(), DR));
   }
 
-  static ChildIteratorType child_end(NodeRef N) {
-    auto InsertVec = N.first->getAddedChildren(N.second, InverseEdge);
+  static auto children(NodeRef N) {
+
     // filter iterator init:
-    auto firstit = make_filter_range(
-        make_range<ExistingChildIterator>({GT::child_end(N.second), N.first},
-                                          {GT::child_end(N.second), N.first}),
-        DeletedEdgesFilter(N.second));
+    auto R = make_range(GT::child_begin(N.second), GT::child_end(N.second));
+    // This lambda is copied into the iterators and persists to callers, ensure
+    // captures are by value or otherwise have sufficient lifetime.
+    auto First = make_filter_range(makeChildRange(R, N.first), [N](NodeRef C) {
+      return !C.first->ignoreChild(N.second, C.second, InverseEdge);
+    });
+
     // new inserts iterator init:
-    auto secondit = make_range<AddNewChildrenIterator>(
-        {InsertVec.end(), N.first}, {InsertVec.end(), N.first});
+    auto InsertVec = N.first->getAddedChildren(N.second, InverseEdge);
+    auto Second = makeChildRange(InsertVec, N.first);
+
+    auto CR = concat<NodeRef>(First, Second);
+    // concat_range contains references to other ranges, returning it would
+    // leave those references dangling - the iterators contain
+    // other iterators by value so they're safe to return.
+    return make_range(CR.begin(), CR.end());
+  }
 
-    return concat_iterator<NodeRef, FilterExistingChildrenIterator,
-                           AddNewChildrenIterator>(firstit, secondit);
+  static auto child_begin(NodeRef N) {
+    return children(N).begin();
   }
+
+  static auto child_end(NodeRef N) {
+    return children(N).end();
+  }
+
+  using ChildIteratorType = decltype(child_end(std::declval<NodeRef>()));
 };
 
 template <typename T, bool B>