[PATCH] D123469: [DebugInfo][llvm-dwarfutil] Combine overlapped address ranges.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

> Sorry, just verified that this will be ok for llvm-gsymutil. The main questions is if this is something that everyone will want from an AddressRanges class. Maybe it would be best to add a default bool argument? See inlined comment

It would probably be better to add such bool argument(or other mechanism) when the real case become clear. So that we implemented it in the most convenient way(bool argument to the constructor, bool argument to the insert method, or probably separate class). It looks like current usages do not want that functionality.



================
Comment at: llvm/include/llvm/ADT/AddressRanges.h:55
 public:
+  using Collection = SmallVector<AddressRange>;
+
----------------
clayborg wrote:
> Does this default to having 1 contained address range when the size integer isn't specified? If so that is good. Might be nice to specify that in the using? Also, this shouldn't be public as uses can now get ahold of the "Collection::iterator" from the insert() and muck with our structure. See inline comment below.
>Does this default to having 1 contained address range when the size integer isn't specified? If so that is good. Might be nice to specify that in the using?

not exactly one range. Following is the comment from SmallVector.h:

```
/// In the absence of a well-motivated choice for the number of inlined
/// elements \p N, it is recommended to use \c SmallVector<T> (that is,
/// omitting the \p N). This will choose a default number of inlined elements
/// reasonable for allocation on the stack (for example, trying to keep \c
/// sizeof(SmallVector<T>) around 64 bytes)
```

if you think it is better to specify SmallVector<T, 1> - will change it.

> Also, this shouldn't be public as uses can now get ahold of the "Collection::iterator" from the insert() and muck with our structure. See inline comment below.

Ok, will hide it under "protected" clause.


================
Comment at: llvm/include/llvm/ADT/AddressRanges.h:70
+  }
+  Collection::iterator insert(AddressRange Range);
   void reserve(size_t Capacity) { Ranges.reserve(Capacity); }
----------------
clayborg wrote:
> No one should be playing with or know about the implementation within this class for the "Collection::iterator". I know you use this iterator in the AddressRangesMap class, maybe we can make a "Collection::iterator insert_impl(AddressRange Range);" that is protected so that the AddressRangesMap can use this, and then leave this function as it was with the possibility of adding a new bool to request that ranges be combined?
> ```
> void insert(AddressRange Range, bool CombineAdjacent = true);
> ```
> The idea would be we always combine overlapping address ranges, so CombineAdjacent would combine when they don't overlap but one of the boundaries matches. It would be also good to document this in the header doc as to what "insert" will do in all cases. Before this was a class only used by llvm-gsymutil, but now with wider adoption it would be good to document.
>No one should be playing with or know about the implementation within this class for the "Collection::iterator". I know you use this iterator in the AddressRangesMap class, maybe we can make a "Collection::iterator insert_impl(AddressRange Range);" that is protected so that the AddressRangesMap can use this, and then leave this function as it was with the possibility of adding a new bool to request that ranges be combined?

Ok, will make insert_impl protected.

>void insert(AddressRange Range, bool CombineAdjacent = true);

>The idea would be we always combine overlapping address ranges, so CombineAdjacent >would combine when they don't overlap but one of the boundaries matches. It would be >also good to document this in the header doc as to what "insert" will do in all cases. Before >this was a class only used by llvm-gsymutil, but now with wider adoption it would be good to >document.

I would suggest to not introduce CombineAdjacent at current moment. gsymutil and dwarfutil do not need that functionality. If it would be needed by some future code then we will add it in the most convenient way. if that is OK, will update a comment accordingly. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123469