[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: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:
> CarlosAlbertoEnciso wrote:
> > psamolysov wrote:
> > > 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.
> > I am afraid I don't follow you. `IntervalSet` is just an alias for the `SmallVector` instance. Do you mean make it `static` as the `printList` method you mentioned in other comment?
> @CarlosAlbertoEnciso Yes, I mean the method `sortIntervals` should be `static` as well as the `printList` one. Technically this should work well. Actually, the `sortIntevals` method expects the `IntervalReferences &IntervalSet` parameter that actually contains the pointers to the `IntervalData` values (as you said, `IntervalReferences` is just an alias to `SmallVector<IntervalData *>`) stored in the tree the method is a member of, but nothing protect the user from calling the method with pointers to `IntevalData`s from another tree and even from an actually dead tree, in other words - with dangling pointers. So, I think it is not a big deal whether the method is `static` or not but because it doesn't use any members of a concrete tree object, it could be better to make the method `static` as well as the `printList` one. Sorry for my previous comment where I said the method should be a non-static member of the class, currently I think it can be a static member but the last word is yours.
@psamolysov Thanks very much for the great explanation.
Making both methods `sortIntervals` and `printList` statics.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125776