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

[David Blaikie]

> 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?



>> 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) {"


>
>
>> 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?


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