<div dir="ltr">Once again, thanks for the quick review :)<div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Aug 30, 2013 at 11:19 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">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></blockquote><div>Yes, that is the plan. </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>
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></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">
- generateDITypeIdentifierMap<br>
* any reason this wouldn't be a ctor of DITypeIdentifierMap (or at<br>
least a member function)?<br></blockquote><div>The current patch does not wrap around DITypeIdentifierMap, that is why generateDITypeIdentifierMap</div><div>is a helper function. If we use a class for DITypeIdentifierMap, the only member will be a DenseMap and</div>
<div>it can have member functions for generating the map and looking up values.</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">
* "DICompositeType Ty = DICompositeType(x);" can be rewritten as<br>
"DICompositeType Ty(x);"<br></blockquote><div>Yes, will make the change.</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">
* map lookup and insertion should be combined, probably simply like 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></blockquote><div>Good deal, one search of the map instead of two. </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>
DwarfDebug.cpp:<br>
- lookupType<br>
* assert should have an explanatory string in it<br></blockquote><div>Yes. </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>
'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></blockquote><div>I will think about this more and chat with you on IRC.</div><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 class=""><div class="h5"><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 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 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 an<br>
> added testing case.<br>
><br>
> The last one is to update testing cases in clang.<br>
><br>
> Thanks,<br>
> Manman<br>
><br>
</div></div></blockquote></div><br></div></div>