[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:300
+                 unsigned Start, unsigned Size, bool HexFormat = true) {
+    const char *Format = HexFormat ? "[0x%08x,0x%08x] " : "[%2d,%2d] ";
+    if (Size) {
----------------
psamolysov wrote:
> 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.
Added your suggested assertion.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:362
+    int MiddleIndex = (PointsBeginIndex + PointsEndIndex) / 2;
+    PointType MiddlePoint = EndPoints[MiddleIndex];
+
----------------
psamolysov wrote:
> 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.
Thanks for your example and explanation.

May be the main problem is that you can create a tree with types that make no sense, like your example. The IntervalTree is expecting fundamental types and pointers.
I will add the following for both `PointT` and `ValueT`:

```
  static_assert(std::is_fundamental<PointT>::value ||
                    std::is_pointer<PointT>::value,
                "PointT must be an fundamental or pointer type");
```
This is added code:

```
...
template <typename T>
using TypeIsValid =
    std::integral_constant<bool, std::is_fundamental<T>::value ||
                                     std::is_pointer<T>::value>;

template <typename PointT, typename ValueT,
          typename DataT = IntervalData<PointT, ValueT>>
class IntervalTree {
  static_assert(TypeIsValid<PointT>::value,
                "PointT must be an fundamental or pointer type");
  static_assert(TypeIsValid<ValueT>::value,
                "ValueT must be an fundamental or pointer type");
...
};
```

With this code, all your comments about `std::move(...)` will be addressed.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:430
+  class find_iterator
+      : public std::iterator<std::forward_iterator_tag, DataType> {
+
----------------
psamolysov wrote:
> `std::iterator` is deprecated in C++17, because the project will use C++17 soon it might make sense to not use a deprecated class for the new code.
Removing `std::iterator` and replacing it with:

```
...
  class find_iterator {
  public:
    using iterator_category = std::forward_iterator_tag;
    using value_type = DataType;
    using difference_type = DataType;
    using pointer = DataType *;
    using reference = DataType &;
...

```


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:592
+  IntervalReferences getContaining(PointType Point) const {
+    IntervalReferences IntervalSet;
+    for (find_iterator Iter = find(Point), E = find_end(); Iter != E; ++Iter)
----------------
psamolysov wrote:
> Should all the query methods contain an assert that the tree has been actually created?
I think it is a good idea. I will add some asserts.


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:625
+    SmallVector<PointType, 4> Points;
+    for (DataType &Data : Intervals) {
+      Points.push_back(Data.left());
----------------
psamolysov wrote:
> 
Added the `const`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125776