<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Sep 3, 2013 at 1:24 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Thanks - few follow-on or outstanding issues:<br>
<br>
Does getFieldAs<DYTypeRef> require an explicit specialization? I<br>
speculated about this on IRC, but it was only a speculation/guess, I<br>
didn't mean to claim I knew it was necessary. Looking at the code, the<br>
implementation of that specialization looks the same as the original<br>
template so can probably be omitted.<br></blockquote><div>getFieldAs used to handle subclass of DIDescriptor, it treats the field as MDNode and constructs a DIDescriptor from it.</div><div>For DITypeRef, the field can be either a MDNode or a MDString, that is why getField instead of getDescriptorField is used</div>
<div>for DITypeRef.</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">
<br>
But if we do need the specialization, could we befriend it (or even<br>
just befriend the whole template) and make the DITypeRef ctor private?<br></blockquote><div>Can we actually befriend a specialization where class DITypeRef is not a template?</div><div>If we befriend the whole template, we also need to make DITypeRef a friend of DIDescriptor so DITypeRef</div>
<div>can access the protected member "DIDescriptor::getFieldAs".</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">
<br>
Remove the "protected" section from DITypeRef - the member should be<br>
private & that's the default in a class anyway.<br></blockquote><div>Will do. </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">
<br>
The documentation comment for DITypeRef discusses implementation<br>
details that aren't really important/relevant to the contract of the<br>
class. I'd probably just say something like "abstracts over direct and<br>
identifier-based metadata type references" or the like.<br></blockquote><div>Are you referring to the comments above the class definition?</div><div>+ /// A thin wrapper to represent reference to a DIType. It wraps around Value*,</div>
<div>+ /// which can be either a MDNode or a MDString, in the latter, MDString</div><div>+ /// specifies the type identifier.</div><div>+ class DITypeRef { </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">
<br>
fieldIsMDStringOrMDNode's name is non-descript in that it describes<br>
the implementation, not the purpose. "isTypeRef" might be<br>
simpler/better (this could even be re-used for the assert in<br>
DITypeRef's ctor if the function can be placed somewhere sufficiently<br>
general - maybe even a static member function of DITypeRef)<br></blockquote><div>I thought about that, the only difference is one handles a field of a MDNode,</div><div>the other handles the actual Value.</div><div>Should we introduce fieldIsTypeRef and isTypeRef?</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">
<br>
Rename "lookupType" to "resolve" for consistency, perhaps?<br>
(problematic to have different verbs/names for the same concept)<br></blockquote><div>Will do.</div><div><br></div><div>Thanks again for the quick review.</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 class=""><div class="h5"><br>
<br>
<br>
<br>
On Tue, Sep 3, 2013 at 12:54 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">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">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">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>><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">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><br></div></div>