[PATCH] D87935: DebugInfo: Filter DWARFv5 TUs out of the debug_info unit list when CUs requested

Wed Sep 23 22:19:23 PDT 2020

dblaikie added a comment.

In D87935#2289415 <https://reviews.llvm.org/D87935#2289415>, @jhenderson wrote:

> In D87935#2285841 <https://reviews.llvm.org/D87935#2285841>, @dblaikie wrote:
>
>> In D87935#2284630 <https://reviews.llvm.org/D87935#2284630>, @jhenderson wrote:
>>
>>> If I had to pick between the two existing options, I think I have a marginal preference for this one, since it requires less code and is less surprising to someone who might not be too familiar with the code - I would expect a list of units to be in offset order naturally, and not expect to have to go through some sort of sorting procedure in the future.
>>
>> Fair - appreciate the perspective. One other wrinkle here is that if we support the "type_units()" API (I don't think anyone's actually using it, so I'd probably delete it for now) it'll need to use a fancy iterator solution too - concat_iterator + filter_iterator to take the v5 type units out of the debug_info units, and concat that with the v4 type units from debug_types.
>
> I'm happy for that little extra complexity. I think it intuitively makes sense, even if it does mean messing about a little in the code.

Fair enough. It's adequate for needs right now - though I'd like to revisit/keep in our mind the idea that maybe these APIs should all be a bit more lightweight/only-as-needed if we can head in that direction. (I'm concerned that to get there may not be readily made incremental, but we'll see - maybe we can pick away at it)

Appreciate the help considering the alternatives here!

================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:166

+  static bool CUOnly(const std::unique_ptr<DWARFUnit> &U) {
+    return !U->isTypeUnit();
----------------
jhenderson wrote:
> Fixes the clang-tidy warning, and also more in keeping with `isTypeUnit`. (Also `isCU` would be acceptable).
Ah, sure thing - (& had a weird bit of leftover where I had two versions of this function (one here and one in DWARFUnit.h) & things were working anyway... which is vaguely unsettling - anyway, removed that duplication in the process of fixing the rename)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87935