[PATCH] Make getUnitForOffset a DWARFUnit method instead of a DWARFContext method.

Frederic Riss friss at apple.com
Thu Sep 11 10:03:16 PDT 2014


================
Comment at: lib/DebugInfo/DWARFContext.cpp:394
@@ -411,9 +393,3 @@
   parseCompileUnits();
-
-  std::unique_ptr<DWARFCompileUnit> *CU =
-      std::lower_bound(CUs.begin(), CUs.end(), Offset, OffsetComparator());
-  if (CU != CUs.end()) {
-    return CU->get();
-  }
-  return nullptr;
+  return DWARFUnit::getUnitForOffset<DWARFCompileUnit>(CUs, Offset);
 }
----------------
dblaikie wrote:
> You shouldn't need to specify UnitType here, should you? Template argument deduction should fire on the first parameter. (similarly at the other call site)
No, type deduction doesn't seem to work here. Just a guess: the implicit conversion from SmallVector to SmallVectorImpl might be an issue?

================
Comment at: lib/DebugInfo/DWARFUnit.h:188
@@ +187,3 @@
+  static UnitType *
+  getUnitForOffset(const SmallVectorImpl<std::unique_ptr<UnitType>> &Units,
+                   uint32_t Offset) {
----------------
dblaikie wrote:
> This doesn't necessarily have to be moved to the DWARFUnit - it could remain in the DWARFContext - though perhaps there's a circular referencing problem that makes this more convenient/possible here rather than where it was originally?
Honestly, when I moved it to a header, I just thought that something comparing Units is better suited in the DWARFUnit class. Now that you mention it there is actually a circular include issue that prevents it from being in the DWARFContext header.

================
Comment at: lib/DebugInfo/DWARFUnit.h:190
@@ +189,3 @@
+                   uint32_t Offset) {
+    auto *Unit = std::upper_bound(Units.begin(), Units.end(), Offset,
+                                  UnitOffsetComparator<UnitType>());
----------------
dblaikie wrote:
> This looks like it's also changing the semantics based on one of your other code reviews - it'd be good to keep these things separate. (similarly I see the getOffset -> getNextUnitOffset change in the UnitOffsetComparator above)
Yes, I mentioned it it the revision log. I needed a working patch to test it out, but if you agree that this is what you want I can easily respin it with the same semantic as before.

================
Comment at: lib/DebugInfo/DWARFUnit.h:216
@@ +215,3 @@
+  DWARFUnit *getUnitForOffset(uint32_t Offset) const override {
+    return DWARFUnit::getUnitForOffset<UnitType>(Units, Offset);
+  }
----------------
dblaikie wrote:
> You shouldn't need to specify UnitType here, should you? Template argument deduction should fire on the first parameter.
As said above, it doesn't work.

http://reviews.llvm.org/D5310






More information about the llvm-commits mailing list