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

[David Blaikie]

On Thu, Sep 11, 2014 at 2:35 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:

>
> On Thu, Sep 11, 2014 at 2:03 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Thu, Sep 11, 2014 at 1:21 PM, Alexey Samsonov <vonosmas at gmail.com>
>> wrote:
>>
>>> ================
>>> Comment at: lib/DebugInfo/DWARFUnit.h:202
>>> @@ +201,3 @@
>>> +template<class UnitType>
>>> +class DWARFUnitImpl : public DWARFUnit {
>>> +protected:
>>> ----------------
>>> See my suggestion in D5264 review thread. You can introduce a new
>>> template class DWARFUnitSection<T>,
>>> which would be a wrapper around SmallVector<std::unique_ptr<T>>, and have
>>> DWARFCompileUnit hold a reference to DWARFUnitSection<DWARFCompileUnit>,
>>> and DWARFTypeUnit hold a reference to DWARFUnitSection<DWARFTypeUnit>.
>>>
>>> Then instead of introducing Unit::getUnitForOffset(), you can have an
>>> (abstract) DWARFUnit::getSection(),
>>
>>
>> Problem here is that getSection would return a different type depending
>> on the DWARFUnit (because it could be a DWARFUnitSection<DWARFCompileUnit>
>> or a DWARFUnitSection<DWARFTypeUnit> - so you'd probably still end up with
>> a CRTP-esque base implementing getUnitForOffset and containing the
>> DWARFUnitSection<DerivedUnitType>&).
>>
>
> Yeah, probably. I'm not opposed to CRTP, I just don't think that unit
> should have direct access to other units from the same section, and "a
> collection of units of the same type from the same section" seems like a
> reasonable abstraction.
>

Fair enough - don't have a problem with that being wrapped up in a minor
abstraction. I don't think it's quite as important - but certainly
different priorities.

- David


>
>
>>
>>
>>> and
>>> DWARFUnitSection::getUnitForOffset().
>>>
>>> http://reviews.llvm.org/D5310
>>>
>>>
>>>
>>
>
>
> --
> Alexey Samsonov
> vonosmas at gmail.com
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140911/4514d6b3/attachment.html>