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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:57
-/// Map LowPC to ObjFileAddressRange.
-using RangesTy = std::map<uint64_t, ObjFileAddressRange>;
 
----------------
clayborg wrote:
> Would we want to include the LowPC in the ObjFileAddressRange struct and then use a std::set<ObjFileAddressRange>?
That is already done, DWARFLinkerAddressRangesMap.h:


```
struct DwarfLinkerAddressRange {
  /// Range LowPC.
  uint64_t LowPC = 0;

  /// Range HighPC.
  uint64_t HighPC = 0;

  ...
}
```


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinkerAddressRangesMap.h:25-29
+  /// Range LowPC.
+  uint64_t LowPC = 0;
+
+  /// Range HighPC.
+  uint64_t HighPC = 0;
----------------
clayborg wrote:
> It would be great to use a ADT "AddressRange" class here instead like something from llvm/include/llvm/DebugInfo/GSYM/Range.h instead of doing things manually.
I agree that using an ADT is a good thing here and DebugInfo/GSYM/Range.h:AddressRange is a good candidate.  But it could not be used as it is. It requires some refactoring. f.e. it should be refactored out of DebugInfo/GSYM. Another thing is that there should be removed dependency on "class FileWriter;", and probably on "DataExtractor". I am OK with doing such refactoring, but would prefer to do it separately from this review. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123469