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

[Greg Clayton via Phabricator via llvm-commits]

clayborg added inline comments.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinkerAddressRangesMap.h:25-29
+  /// Range LowPC.
+  uint64_t LowPC = 0;
+
+  /// Range HighPC.
+  uint64_t HighPC = 0;
----------------
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.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinkerAddressRangesMap.h:37
+  bool contains(uint64_t Address) {
+    return LowPC <= Address && HighPC > Address;
+  }
----------------
Might be a bit clearer as suggested, or if we use a AddressRange class it would be:
```
return Range.contains(Address);
```


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinkerAddressRangesMap.h:102
+  void forEach(
+      llvm::function_ref<void(const DwarfLinkerAddressRange &)> Handler) const {
+    for (AddressIntervals::const_iterator It = Ranges.begin();
----------------
If the "Handler" function returns a bool, you can stop early if it returns false, or keep iterating if it returns true. No need if you don't think it would be useful


================
Comment at: llvm/unittests/DWARFLinker/DWARFLinkerAddressRangesMapTest.cpp:1
+//===- DWARFLinkerAddressRangesMapTest.cpp --------------------------------===//
+//
----------------
If we decode to possibly use something like the llvm/include/llvm/DebugInfo/GSYM/Range.h classes, those all have unit tests.


================
Comment at: llvm/unittests/DWARFLinker/DWARFLinkerAddressRangesMapTest.cpp:34
+  ASSERT_FALSE(RangesMap.empty());
+
+  RangesMap.forEach([&](const DwarfLinkerAddressRange &Range) {
----------------
If we change the addRange function to check using <=, add test for that here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123469