[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:362
+    int MiddleIndex = (PointsBeginIndex + PointsEndIndex) / 2;
+    PointType MiddlePoint = EndPoints[MiddleIndex];
+
----------------
CarlosAlbertoEnciso wrote:
> 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.
Good idea! Thank you! Yes, having the constraint on both `PointT` and `ValueT` addresses all my comments about references and `std::move`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125776