<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 17 Sep 2014, at 08:11, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Tue, Sep 16, 2014 at 10:47 PM, Frederic Riss <span dir="ltr" class=""><<a href="mailto:friss@apple.com" target="_blank" class="">friss@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The Units method was suggested by Alexey I have no strong feeling either way. With this patch applied, the inheritance of SmallVector isn't necessary anymore, I didn't fold the move from inheritance to composition in this patch, but it could be done.<br class=""></blockquote><div class=""><br class=""></div><div class="">Yeah, I saw from his CR feedback.<br class=""><br class="">To be honest, since seeing this patch I've been dabbling with some changes of my own to demonstrate a possible direction.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The next step for me is to factor the the parsing code into this class. </blockquote><div class=""><br class=""></div><div class="">The thing is, I think factoring more of the logic, such as the parsing, into this class will actually eventually remove the need to expose the container-like API of this class at all. <br class=""><br class="">My patch is probably a bit over the top, but for demonstration, I've included it (locally I've got it broken down into smaller steps).<br class=""><br class="">The only thing missing to make DWARFUnitSection not expose begin/end (and thus, not expose any container-like API) is to </div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">There is an intermediate step though: the actual code is semantically wrong when it comes to type units. Type units might come from different sections, but we put them all in the same DWARFUnitSection object. </blockquote><div class=""><br class=""></div><div class="">Why is this a problem? Oh, I suppose getUnitForOffset doesn't work so well when it's not a contiguous non-overlapping range?<br class=""><br class="">Fair enough.<br class=""><br class="">Note that it's only non-fission type units that come from different sections. (& I don't know how comdat sections look once they're all linked and deduplicated - so I guess it might be only type units in object files (not executables or .dwo files) that are in their own sections)</div></div></div></div></div></blockquote><div><br class=""></div><div>Yes, this will be the case in object files. And the issue really is getUnitForOffset getting confused (after all the thinking we put into this one, that would be a pity, wouldn’t it? :-))</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Once this is fixed, we can move parsing into DWARFUnitSection and remove at least the addUnit method.<br class=""></blockquote><div class=""><br class=""></div><div class="">Fair enough - the attached patch just moves it as-is, which is no worse than the current state, I don't think.</div><div class=""><div class="gmail_chip gmail_drive_chip" style="width:396px;height:18px;max-height:18px;background-color:#f5f5f5;padding:5px;color:#222;font-family:arial;font-style:normal;font-weight:bold;font-size:13px;border:1px solid #ddd;line-height:1"><a href="https://docs.google.com/file/d/0B0jpkch3iC_7ZXRwR1d5cGFOcVk/edit?usp=drive_web" target="_blank" style="display:inline-block;max-width:366px;overflow:hidden;text-overflow:ellipsis;white-space:nowrap;text-decoration:none;padding:1px 0;border:none" class=""><img style="vertical-align: bottom; border: none;" src="https://ssl.gstatic.com/docs/doclist/images/icon_10_generic_list.png" class=""> <span dir="ltr" style="color:#15c;text-decoration:none;vertical-align:bottom" class="">DWARFUnitSection.diff</span></a></div></div></div></div></div></div></blockquote><div><br class=""></div><div>I don’t have permission to access that file. I guess it’s pretty similar to what my topic branch looks like.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class="">There's some extra tidying up I could do on that patch (& locally I've got it more granular) - but I figured it might be some use to you and/or Alexey to see where it might go. Like your patch, mine doesn't actually remove the inheritance - it just demonstrates (by making the inheritance private and not exposing any container-like operations or typedefs) that it's no longer relied upon by anything other than the implementation. At which point it's fairly easy to refactor it to composition.<br class=""></div></div></div></div></div></blockquote><div><br class=""></div><div>To test my patch, this is exactly the approach I used. I moved to protected inheritance and chose the APIs that would minimise the API surface while not requiring too much of a refactoring. The goal was simply to produce an easy to review ‘wrapper’ patch, and leave the bigger methods for later. I thought this is the approach you were suggesting. And yes, this introduces a bit of API that is meant to disappear soon, but it would have allowed me to propose the inheritance removal patch in parallel to more functionality refactoring ones.</div><div><br class=""></div><div>Fred</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">(personally, I think moving the parsing and probably the dumping support there makes sense - the rest of it... I'd be just as happy to have begin/end exposed on the DWARFUnitSection and keep the last couple (the line table and arange handling) where they were, probably)<br class=""><br class="">Alexey - you mentioned preferring not to make DWARFUnitSection not a container itself (but rather to have a "units" member on it) - I don't feel strongly, but we do have at least some prior art/tradition of having things /be/ containers in LLVM (Modules are containers of functions, functions are containers of basic blocks, basic blocks are containers of instructions, etc) so it's not a problem to do that & in some cases I think it's rather nice. Interested to hear your thoughts/perspective.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Once this is done a bunch of accessors from the context can go away also.<br class="">
<br class="">
<a href="http://reviews.llvm.org/D5369" target="_blank" class="">http://reviews.llvm.org/D5369</a><br class="">
<br class="">
<br class="">
</blockquote></div><br class=""></div></div>
</div></blockquote></div><br class=""></body></html>