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

[Manman Ren]

In r190081 and r190082.

Manman


On Thu, Sep 5, 2013 at 10:44 AM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
>
> 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/145f2cb5/attachment.html>