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

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

Ok, if we just protect the Collection::iterator stuff in AddessRanges, this will be good to go.



================
Comment at: llvm/include/llvm/ADT/AddressRanges.h:55
 public:
+  using Collection = SmallVector<AddressRange>;
+
----------------
avl wrote:
> 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.
Ok, leave SmallVector<T> as is, I was unaware of this nice feature.


================
Comment at: llvm/include/llvm/ADT/AddressRanges.h:70
+  }
+  Collection::iterator insert(AddressRange Range);
   void reserve(size_t Capacity) { Ranges.reserve(Capacity); }
----------------
avl wrote:
> 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. 
Fine to add it later, maybe  add a comment letting users knows that any adjacent address ranges will be combined. I am not sure what people would expect here, but you are correct in that all usage of this wants adjacent ranges to be combined, so all good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123469