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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:36
+// Synopsis:
+//   PointT objects are mapped to ValueT objects.
+//
----------------
probinson wrote:
> 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*` ?)
> 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. "

Taking your suggested synopsis.

> 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*` ?)

Added the following restrictions:
`ValueT: fundamental or pointer types.`
`PointT: fundamental types.`



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



================
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.
----------------
probinson wrote:
> 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.)
> Let's not use the word "ranges" to describe a type, as that means something else in C++. "interval endpoints type" maybe?

Changed `ranges` to `interval endpoints`.


================
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;
----------------
probinson wrote:
> `assert(Left < Right)` ?  Or `assert(Left <= Right)` if [x, x] intervals are allowed.
> Maybe you check this elsewhere and I didn't see it.
Added
`assert(Left <= Right && "'Left' must be less or equal that 'Right'");`


================
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>;
----------------
probinson wrote:
> This could use `std::bool_constant`
Changed to use `std::bool_constant`.


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


================
Comment at: llvm/unittests/ADT/IntervalTreeTest.cpp:82
+
+  EXPECT_EQ(Tree.empty(), true);
+  Tree.clear();
----------------
probinson wrote:
> psamolysov wrote:
> > 
> +1
Changed all ocurrences:
`EXPECT_EQ(cond, true);` --> `EXPECT_TRUE(cond);`


================
Comment at: llvm/unittests/ADT/IntervalTreeTest.cpp:84
+  Tree.clear();
+  EXPECT_EQ(Tree.empty(), true);
+
----------------
probinson wrote:
> psamolysov wrote:
> > 
> +1
Changed all ocurrences:
`EXPECT_EQ(cond, true);` --> `EXPECT_TRUE(cond);`


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

https://reviews.llvm.org/D125776