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

[Pavel Samolysov via Phabricator via llvm-commits]

psamolysov added inline comments.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:300
+                 unsigned Start, unsigned Size, bool HexFormat = true) {
+    const char *Format = HexFormat ? "[0x%08x,0x%08x] " : "[%2d,%2d] ";
+    if (Size) {
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > 
> I belive the assertion condition should be:
> `assert(Start + Size < IntervalSet.size() && "Start + Size must be in bounds of the IntervalSet");`
Sure, sorry for the typo in the suggestion.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:362
+    int MiddleIndex = (PointsBeginIndex + PointsEndIndex) / 2;
+    PointType MiddlePoint = EndPoints[MiddleIndex];
+
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > CarlosAlbertoEnciso wrote:
> > > psamolysov wrote:
> > > > To not copy the `PointType` of unknown size?
> > > I am not sure if I understand the issue. A typical case to create the IntervalTree would be:
> > > 
> > > ```
> > > using UUPoint = unsigned; // Interval range type.
> > > using UUValue = unsigned; // Mapped value type.
> > > 
> > > using UUTree = IntervalTree<UUPoint, UUValue>;
> > > ```
> > > with `PointType` being `unsigned`
> > > 
> > @CarlosAlbertoEnciso Thank you for the response. I though because `PointType` is a template parameter (an alias for `PointT` actually) it may be any type. If you are sure the type will be `unsigned` in a typical case, there is no reason to use a const reference for `unsigned`.
> I think we get confused.
> - `PointType` is a template parameter; alias for `PointT` and can be any type.
> - I used `unsigned` as an example to show how the `IntervalTree` can be used.
> 
> In your original comment, you wrote:
> `To not copy the PointType of unknown size?`
> 
> I am not quite sure about your question.
I mean that one can create the following `IntervalTree`:

```
using UUPoint = std::array<unsigned, 128>; // Just an example, this maybe makes no sense but it is possible.
using UUValue = unsigned; // Mapped value type.

using UUTree = IntervalTree<UUPoint, UUValue>;
```

And in **this** tree your original line

```
PointType MiddlePoint = EndPoints[MiddleIndex];
```

means that the whole array of 128 `unsigned`s will be copied into `MiddlePoint` even though the value is already in the `EndPoints` and can be accessible by a const reference. On the other hand, if `PointType` is just an `unsigned`, the copy is better than the reference creation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125776