[PATCH] D125776: [llvm-debuginfo-analyzer] 01 - Interval tree

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:36
+// Synopsis:
+//   PointT objects are mapped to ValueT objects.
+//
----------------
I think this description sounds too much like what std::map does.  Also I think it's worth mentioning that they are closed intervals.
Suggested synopsis: "Closed intervals delimited by PointT objects are mapped to ValueT objects. "

Also, should state the restriction enforced by IntervalData, i.e., that PointT and ValueT must be fundamental or pointer types.  (A pointer type for PointT would be risky, as pointer comparison results are "unspecified" if the two pointers address different objects that are not in the same array.  The only really safe pointer type for PointT would be `void*`. Maybe you want to disallow pointer types? Or only `void*` ?)


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:77
+// [74, 80] <- (7480),    [15, 40] <- (1540),    [43, 45] <- (4345),
+// [50, 75] <- (5075),    [10, 85] <- (1085)
+//
----------------
Are you allowed to have an interval like [12, 12]? I don't see any examples like that here.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:193
+/// associated \a Value.
+/// \a PointT corresponds to the interval ranges type.
+/// \a ValueT corresponds to the interval value type.
----------------
Let's not use the word "ranges" to describe a type, as that means something else in C++. "interval endpoints type" maybe?
(In other commentary, "ranges" can be a reasonable word to use.)


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:208
+  IntervalData(PointType Left, PointType Right, ValueType Value)
+      : Left(Left), Right(Right), Value(Value) {}
+  virtual ~IntervalData() = default;
----------------
`assert(Left < Right)` ?  Or `assert(Left <= Right)` if [x, x] intervals are allowed.
Maybe you check this elsewhere and I didn't see it.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:236
+using TypeIsValid =
+    std::integral_constant<bool, std::is_fundamental<T>::value ||
+                                     std::is_pointer<T>::value>;
----------------
This could use `std::bool_constant`


================
Comment at: llvm/unittests/ADT/IntervalTreeTest.cpp:143
+
+using UUPoint = unsigned; // Interval range type.
+using UUValue = unsigned; // Mapped value type.
----------------
Interval endpoint type.


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


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


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

https://reviews.llvm.org/D125776