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

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

In D123469#3533876 <https://reviews.llvm.org/D123469#3533876>, @avl wrote:

> @clayborg Greg, Could you check&accept this review directly, please?
>
> After requested refactoring is done(https://reviews.llvm.org/D123469?id=423119#inline-1190512)
> the AddressRanges class was changed. It combines the ranges like this:
>
> [0x100,0x200) and [0x200, 0x300) -> [0x100, 0x300)
>
> It can affect llvm-gsymutil. I think it should be OK to combine ranges like this. Please check that behavior.

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



================
Comment at: llvm/include/llvm/ADT/AddressRanges.h:55
 public:
+  using Collection = SmallVector<AddressRange>;
+
----------------
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.


================
Comment at: llvm/include/llvm/ADT/AddressRanges.h:70
+  }
+  Collection::iterator insert(AddressRange Range);
   void reserve(size_t Capacity) { Ranges.reserve(Capacity); }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123469