[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:151
+// contain the intervals described by IL and IR. Each node will contain an
+// index into that vector (global bucket), to indicate the beginning of the
+// intervals assigned to the node.
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > Just a nit. Personally I didn't manage to get how the intervals actually stored in the node until I had a look at the source code. Maybe the "that vector (global bucket)" should be introduced a little bit before?
> May be before the line `// The following is the output from print():` ?
> 
Moved comments before the `// The following is the output from print():`


================
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:
> 
I belive the assertion condition should be:
`assert(Start + Size < IntervalSet.size() && "Start + Size must be in bounds of the IntervalSet");`


================
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);
----------------
psamolysov wrote:
> 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?
Yes. I will rename all the lambdas across all patches.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125776