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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:119
+//   IL: Intervals to the left (in ascending order by beginning point).
+//   IR: Intervals to the right (in ascending order by ending point).
+//
----------------
psamolysov wrote:
> It is described as the intervals are stored above but I see in the example they are printed in the same order, for IR - in the descending order by ending point.
You are rigth. The comment should read `in descending order by ending point`.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:151
+// contain the intervals described by IL and IR. Each node will contain an
+// index into that vector (global bucket), to indicate the beginning of the
+// intervals assigned to the node.
----------------
psamolysov wrote:
> Just a nit. Personally I didn't manage to get how the intervals actually stored in the node until I had a look at the source code. Maybe the "that vector (global bucket)" should be introduced a little bit before?
May be before the line `// The following is the output from print():` ?



================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:175
+//    Point: a target point to be tested for inclusion in any interval.
+//    Returns vector will all the intervals containing the target point.
+//
----------------
psamolysov wrote:
> 
Changed `will` to `with`.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:224
+  /// This is Left <= Point <= Right for closed intervals.
+  bool contain(const PointType &Point) const {
+    return left(Point) && right(Point);
----------------
psamolysov wrote:
> Maybe to use `contains` (with S) as the method's name? For example, the standard library contains classes with methods with this name: https://en.cppreference.com/w/cpp/string/basic_string_view/contains
Sounds good. Changing `contain` to `contains`.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:315
+    const char *Format = HexFormat ? "MP:0x%08x " : "MP:%2d ";
+    auto printNodeData = [&](StringRef Text, IntervalReferences &IntervalSet) {
+      OS << format("%5d: ", Level);
----------------
psamolysov wrote:
> Just a nit. By the coding convention all names of the variables should start from a capital letter, I'm not sure whether lambdas are exception here?
The current discussion:
https://discourse.llvm.org/t/naming-lambdas-in-llvm-coding-standards/62689/6

It seems the consensus is to treat them as variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125776