<div dir="ltr">Attaching updated patch that should handle the review requests.<div><br></div><div>Thanks,</div><div>Manman</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Aug 30, 2013 at 3:13 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Fri, Aug 30, 2013 at 3:10 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>

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