[Type Uniquing Patch] Use identifier instead of MDNode for getClassType of ptr_to_member.

[David Blaikie]

On Thu, Sep 5, 2013 at 10:26 AM, Manman Ren <manman.ren at gmail.com> wrote:

>
>
>
> On Thu, Sep 5, 2013 at 10:10 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>   The comment added "getClassType()" can probably be omitted. It
>>>> documents DITypeRef's semantics (& implementation details I mentioned
>>>> weren't really public documentation for the type, let alone functions that
>>>> return objects of that type) which are out of place here. We won't want to
>>>> add such a documentation comment to every function that returns or handles
>>>> a DITypeRef - we know what they are or we can look at the implementation (&
>>>> comments, doc or otherwise) to find out, rather than redundantly
>>>> documenting it at every use.
>>>>
>>>
>>> I forgot to update the comments after wrapping Value* in DITypeRef.
>>> Updated in the attached patch.
>>>
>>
>> I'd still skip the "as a DITypeRef" - that's obvious from the signature,
>> there's no need to duplicate that information in the comment. And if
>> "ClassType" is unclear and "base type" is more clear, perhaps we should
>> just rename the function so its purpose is obvious from the name & does not
>> require a comment at all?
>>
> Updated the comments again:
> /// The class type pointer-to-member is pointing to members of.
>

This isn't so much a task of finding a comment that adds value, but making
the code understandable (with comments if necessary).

Personally I'd just omit this for now/in this commit - since it's
orthogonal to the change you're trying to make. If anything, the change to
"base type" (or just "getBase") might be appropriate in a separate commit.
I haven't thought about this enough/don't want to bother with it right now
in this change.


>
>
>>
>>
>>
>>>> Is there any reason to build the map of all the types in /all/ the CUs?
>>>> rather than just building up a map for each CU as we visit it? (in the
>>>> scope of the for loop a few lines below the "generateDITypeIdentifierMap"
>>>> call)
>>>>
>>>  I don't see a for loop below the "generateDITypeIdentifierMap" call.
>>> Are you referring to the implementation of generateDITypeIdentifierMap :)
>>> When I search for "for (", they are all in the body of
>>> generateDITypeIdentifierMap.
>>>
>>
>> The for loop proceeding the call to generateDITypeIdentifierMap:
>>
>> "  // Emit initial sections so we can reference labels later.
>>   emitSectionLabels();
>>
>>   for (unsigned i = 0, e = CU_Nodes->getNumOperands(); i != e; ++i) {"
>>
> Oh, I was checking the patch only. To me, it seems to be cleaner to have a
> simple call to construct the Map, since the Map is shared across CUs.
> Also if we have a def in one CU and a declaration in another, we prefer
> the def. The earlier traversed CU can use the def if we build the Map in
> one call.
>

Fair enough.


>
>
>>
>>>
>>>
>>>> Why does DIDescriptor befriend DITypeRef?
>>>>
>>> If we don't, DITypeRef can't befriend the protected member function of
>>> DIDescriptor (DIDescriptor::getFieldAs).
>>>
>>
>> Hmm, OK - can't think of anything particularly better to do there, I
>> suppose. Maybe a comment at the friend to explain why it's befriended?
>>
> Yes, add comments.
>     // Befriends DITypeRef so DITypeRef can befriend the protected member
>     // function: getFieldAs<DITypeRef>.
>

LGTM.

Please commit.


>
> Thanks,
> Manman
>
>>
>>
>>>
>>> Thanks,
>>> Manman
>>>
>>>>
>>>>
>>>> - David
>>>>
>>>>
>>>>
>>>>
>>>>> Manman
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Sep 3, 2013 at 12:54 PM, Manman Ren <manman.ren at gmail.com>
>>>>>> wrote:
>>>>>> > Attaching updated patch that should handle the review requests.
>>>>>> >
>>>>>> > Thanks,
>>>>>> > Manman
>>>>>> >
>>>>>> >
>>>>>> > On Fri, Aug 30, 2013 at 3:13 PM, David Blaikie <dblaikie at gmail.com>
>>>>>> wrote:
>>>>>> >>
>>>>>> >> On Fri, Aug 30, 2013 at 3:10 PM, Manman Ren <manman.ren at gmail.com>
>>>>>> wrote:
>>>>>> >> > Once again, thanks for the quick review :)
>>>>>> >> >
>>>>>> >> >
>>>>>> >> > On Fri, Aug 30, 2013 at 11:19 AM, David Blaikie <
>>>>>> dblaikie at gmail.com>
>>>>>> >> > wrote:
>>>>>> >> >>
>>>>>> >> >> Looks generally reasonable.
>>>>>> >> >>
>>>>>> >> >> I take it you're using pointer_to_member's base type as the
>>>>>> first
>>>>>> >> >> field example & we'll see similar, smaller patches (not adding
>>>>>> any
>>>>>> >> >> infrastructure, just porting another field over) for each field
>>>>>> from
>>>>>> >> >> here on out? It seems like a nice, small example - thanks for
>>>>>> that.
>>>>>> >> >
>>>>>> >> > Yes, that is the plan.
>>>>>> >> >>
>>>>>> >> >>
>>>>>> >> >> lib/IR/DebugInfo.cpp
>>>>>> >> >>  - DIDerivedType::Verify
>>>>>> >> >>    * we should have a single function that does
>>>>>> >> >> "fieldIsMDNode/fieldIsMDString" because we're going to use that
>>>>>> test
>>>>>> >> >> for every type field
>>>>>> >> >
>>>>>> >> > Will do.
>>>>>> >> >>
>>>>>> >> >>  - generateDITypeIdentifierMap
>>>>>> >> >>    * any reason this wouldn't be a ctor of DITypeIdentifierMap
>>>>>> (or at
>>>>>> >> >> least a member function)?
>>>>>> >> >
>>>>>> >> > The current patch does not wrap around  DITypeIdentifierMap,
>>>>>> that is why
>>>>>> >> > generateDITypeIdentifierMap
>>>>>> >> > is a helper function. If we use a class for DITypeIdentifierMap,
>>>>>> the
>>>>>> >> > only
>>>>>> >> > member will be a DenseMap and
>>>>>> >> > it can have member functions for generating the map and looking
>>>>>> up
>>>>>> >> > values.
>>>>>> >>
>>>>>> >> Ah, right - yeah, it's probably not quite worth a separate type,
>>>>>> then.
>>>>>> >> Optionally - we could return a DITypeIdentifierMap by value rather
>>>>>> >> than passing one in to populate, if the initialization happens at
>>>>>> the
>>>>>> >> point of construction to take advantage of NRVO (or if DenseMap has
>>>>>> >> move-related overloads in C++11 mode, that'd suffice too).
>>>>>> >>
>>>>>> >> >
>>>>>> >> >>    * "DICompositeType Ty = DICompositeType(x);" can be
>>>>>> rewritten as
>>>>>> >> >> "DICompositeType Ty(x);"
>>>>>> >> >
>>>>>> >> > Yes, will make the change.
>>>>>> >> >
>>>>>> >> >>
>>>>>> >> >>    * map lookup and insertion should be combined, probably
>>>>>> simply like
>>>>>> >> >> this:
>>>>>> >> >>      pair<iterator, bool> p = Map.insert(make_pair(TypeId,
>>>>>> Ty)); //
>>>>>> >> >> try to insert
>>>>>> >> >>      if (!p.second && !Ty.isForwardDecl()) // if we didn't
>>>>>> insert
>>>>>> >> >> because the element was already there
>>>>>> >> >>        p.first->second = Ty; // replace whatever we had
>>>>>> (declaration
>>>>>> >> >> or definition) with a definition
>>>>>> >> >>      (pseudocode, comments for illustration only)
>>>>>> >> >
>>>>>> >> > Good deal, one search of the map instead of two.
>>>>>> >> >>
>>>>>> >> >>
>>>>>> >> >> DwarfDebug.cpp:
>>>>>> >> >>   - lookupType
>>>>>> >> >>     * assert should have an explanatory string in it
>>>>>> >> >
>>>>>> >> > Yes.
>>>>>> >> >>
>>>>>> >> >>
>>>>>> >> >> 'getClassType' - this is going to become pervasive in every DI*
>>>>>> >> >> function that returns DIType, right? I think it might be
>>>>>> helpful to
>>>>>> >> >> add an opaque type here so we're not just referring to any
>>>>>> Value.
>>>>>> >> >> Either something totally opaque, a class that befriends the
>>>>>> >> >> DIDescriptor class (for construction) and the map (for lookup) &
>>>>>> >> >> otherwise has no public members except simple copying, etc. Or
>>>>>> perhaps
>>>>>> >> >> a PointerUnion of MDNode/MDString - this is unnecessary, of
>>>>>> course,
>>>>>> >> >> since the Value hierarchy already knows which object it is, so
>>>>>> there's
>>>>>> >> >> no need to discriminate the pointer in actuality...
>>>>>> >> >
>>>>>> >> > I will think about this more and chat with you on IRC.
>>>>>> >> >
>>>>>> >> > Thanks,
>>>>>> >> > Manman
>>>>>> >> >
>>>>>> >> >>
>>>>>> >> >>
>>>>>> >> >> On Fri, Aug 30, 2013 at 10:40 AM, Manman Ren <
>>>>>> manman.ren at gmail.com>
>>>>>> >> >> wrote:
>>>>>> >> >> > Also add a typedef for DITypeIdentifierMap and a helper
>>>>>> >> >> > generateDITypeIdentifierMap in DebugInfo.h. In
>>>>>> DwarfDebug.cpp, we
>>>>>> >> >> > keep
>>>>>> >> >> > a DITypeIdentifierMap and call generateDITypeIdentifierMap to
>>>>>> >> >> > actually
>>>>>> >> >> > populate the map. DwarfDebug also has a helper function
>>>>>> lookupType
>>>>>> >> >> > that
>>>>>> >> >> > finds the type MDNode for a given Value (either an identifier
>>>>>> in
>>>>>> >> >> > MDString
>>>>>> >> >> > or the actual MDNode).
>>>>>> >> >> >
>>>>>> >> >> > Verifier is updated accordingly.
>>>>>> >> >> >
>>>>>> >> >> > 3 patches are attached:
>>>>>> >> >> > The first one is a simple refactoring to add getField to
>>>>>> DIDescriptor
>>>>>> >> >> > so
>>>>>> >> >> > we
>>>>>> >> >> > can use it to return Value* for getClassType.
>>>>>> >> >> >
>>>>>> >> >> > The second one is the actual change to DIBuilder and
>>>>>> DwarfDebug etc
>>>>>> >> >> > with
>>>>>> >> >> > an
>>>>>> >> >> > added testing case.
>>>>>> >> >> >
>>>>>> >> >> > The last one is to update testing cases in clang.
>>>>>> >> >> >
>>>>>> >> >> > Thanks,
>>>>>> >> >> > Manman
>>>>>> >> >> >
>>>>>> >> >
>>>>>> >> >
>>>>>> >
>>>>>> >
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130905/34c3b8ec/attachment.html>