[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:256-258
+    PointType middle() { return MiddlePoint; }
+    unsigned start() { return BucketIntervalsStart; }
+    unsigned size() { return BucketIntervalsSize; }
----------------
psamolysov wrote:
> Should these three methods be marked as `const`?
Marked as `const`.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:355
+    // The intervals in S_left and S_right are recursively divided in the
+    // same manner until there are no intervals left.
+
----------------
psamolysov wrote:
> probinson wrote:
> > psamolysov wrote:
> > > The word `left` has different meaning in the comment: as opposite to the right and `while there is at least one interval`. English is not my native language, and the first time I read the phrase as "there are no intervals in the left side". Could you rewrite the comment a bit?
> > Maybe "no intervals remaining" would avoid the ambiguity.
> @probinson Sounds good
The comment will read as `... until there are no intervals remaining.`.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:539
+    const DataType *operator->() const {
+      return (Point <= Node->middle())
+                 ? (*AscendingBuckets)[Node->start() + Index]
----------------
psamolysov wrote:
> There is a code duplication between `operator->` and `operator*`. It may make sense to extract the body of the `operator->` into a private method (`current()`, for example) that returns a pointer and dereference this method's return value in the `operator*`.
Good point. Refactored the common code into a private method `current()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125776