<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Sep 5, 2013 at 10:26 AM, Manman Ren <span dir="ltr"><<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@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 dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div class="im">On Thu, Sep 5, 2013 at 10:10 AM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote">
<div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
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.<br>
</div></div></div></div></blockquote><div><br></div></div><div>I forgot to update the comments after wrapping Value* in DITypeRef. Updated in the attached patch. </div></div></div></div></blockquote><div><br></div></div>
<div>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?</div>
</div></div></div></blockquote></div><div>Updated the comments again:</div><div>/// The class type pointer-to-member is pointing to members of.</div></div></div></div></blockquote><div><br></div><div>This isn't so much a task of finding a comment that adds value, but making the code understandable (with comments if necessary).<br>
<br>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div><br>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)</div>
</div></div></div></blockquote></div><div> I don't see a for loop below the "generateDITypeIdentifierMap" call. Are you referring to the implementation of generateDITypeIdentifierMap :)</div><div>When I search for "for (", they are all in the body of generateDITypeIdentifierMap.</div>
</div></div></div></blockquote><div><br></div></div><div>The for loop proceeding the call to generateDITypeIdentifierMap:<br><br>" // Emit initial sections so we can reference labels later.</div><div> emitSectionLabels();</div>
<div><br></div><div> for (unsigned i = 0, e = CU_Nodes->getNumOperands(); i != e; ++i) {"</div></div></div></div></blockquote></div><div>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.</div>
<div>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.</div></div></div></div></blockquote><div><br></div><div>Fair enough.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>
<br>Why does DIDescriptor befriend DITypeRef?</div></div></div></div></blockquote></div><div>If we don't, DITypeRef can't befriend the protected member function of DIDescriptor (DIDescriptor::getFieldAs).</div></div>
</div></div></blockquote></div><div><br>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?<br></div></div></div></div></blockquote>
</div><div>Yes, add comments.</div><div> // Befriends DITypeRef so DITypeRef can befriend the protected member</div><div> // function: getFieldAs<DITypeRef>. </div></div></div></div></blockquote><div><br></div>
<div>LGTM.<br><br>Please commit.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<br></div><div>Thanks,</div><div>Manman</div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>
</div><div>Thanks,</div><div>Manman </div><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div><span><font color="#888888"><br><br>- David<br><br><br></font></span></div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div>Manman</div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div><br>
<br>
<br>
<br>
On Tue, Sep 3, 2013 at 12:54 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@gmail.com</a>> wrote:<br>
> Attaching updated patch that should handle the review requests.<br>
><br>
> Thanks,<br>
> Manman<br>
><br>
><br>
> On Fri, Aug 30, 2013 at 3:13 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> On Fri, Aug 30, 2013 at 3:10 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com" target="_blank">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" target="_blank">dblaikie@gmail.com</a>><br>
>> > 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<br>
>> > only<br>
>> > member will be a DenseMap and<br>
>> > it can have member functions for generating the map and looking up<br>
>> > values.<br>
>><br>
>> 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>
>><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" target="_blank">manman.ren@gmail.com</a>><br>
>> >> wrote:<br>
>> >> > Also add a typedef for DITypeIdentifierMap and a helper<br>
>> >> > generateDITypeIdentifierMap in DebugInfo.h. In DwarfDebug.cpp, we<br>
>> >> > keep<br>
>> >> > a DITypeIdentifierMap and call generateDITypeIdentifierMap to<br>
>> >> > actually<br>
>> >> > populate the map. DwarfDebug also has a helper function lookupType<br>
>> >> > 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<br>
>> >> > 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<br>
>> >> > 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>
><br>
><br>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>