[PATCH] D125776: [llvm-dva] 01 - Interval tree

[Pavel Samolysov via Phabricator via llvm-commits]

psamolysov added inline comments.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:580
+    IntervalsRight.clear();
+    EndPoints.clear();
+  }
----------------
psamolysov wrote:
> Maybe the `References` vector also should be cleared?
Sorry, I see the `References` vector are cleared at the end of the `create()` method.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:601
+  /// Descending: return the intervals with the biggest at the front.
+  void sortIntervals(IntervalReferences &IntervalSet, Sorting Sort) const {
+    std::sort(IntervalSet.begin(), IntervalSet.end(),
----------------
psamolysov wrote:
> This method is for sorting already returned interval set, so this is just a helper and it looks like the method can be a static member of the class.
Hm... maybe I get it wrong and because the `IntervalSet` contains only pointers inside the instance of the tree, the method should be a member of the class, not a static member.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:625
+    SmallVector<PointType, 4> Points;
+    for (DataType &Data : Intervals) {
+      Points.push_back(Data.left());
----------------



================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:628
+      Points.push_back(Data.right());
+      References.emplace_back(std::addressof(Data));
+    }
----------------
To be consistent with 2 lines above.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:657
+               ? find_end()
+               : find_iterator(&IntervalsLeft, &IntervalsRight, Root, Point);
+  }
----------------



================
Comment at: llvm/unittests/ADT/IntervalTreeTest.cpp:82
+
+  EXPECT_EQ(Tree.empty(), true);
+  Tree.clear();
----------------



================
Comment at: llvm/unittests/ADT/IntervalTreeTest.cpp:84
+  Tree.clear();
+  EXPECT_EQ(Tree.empty(), true);
+
----------------



================
Comment at: llvm/unittests/ADT/IntervalTreeTest.cpp:97
+  Intervals = Tree.getContaining(Point);
+  EXPECT_EQ(Intervals.empty(), true);
+
----------------
I see you use `ASSERT_TRUE` with `==` and `EXPECT_EQ(..., true)` many times below. So, it means that this usage is expected.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125776/new/

https://reviews.llvm.org/D125776