[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:315
+    const char *Format = HexFormat ? "MP:0x%08x " : "MP:%2d ";
+    auto printNodeData = [&](StringRef Text, IntervalReferences &IntervalSet) {
+      OS << format("%5d: ", Level);
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > Just a nit. By the coding convention all names of the variables should start from a capital letter, I'm not sure whether lambdas are exception here?
> The current discussion:
> https://discourse.llvm.org/t/naming-lambdas-in-llvm-coding-standards/62689/6
> 
> It seems the consensus is to treat them as variables.
If we treat lambdas as variables, we should use leading upper case characters like for other variables. Would you like to rename all the lambdas?


================
Comment at: llvm/include/llvm/ADT/IntervalTree.h:362
+    int MiddleIndex = (PointsBeginIndex + PointsEndIndex) / 2;
+    PointType MiddlePoint = EndPoints[MiddleIndex];
+
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125776