<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 16, 2014 at 10:47 PM, Frederic Riss <span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span> wrote:<br><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></blockquote><div><br></div><div>Yeah, I saw from his CR feedback.<br><br>To be honest, since seeing this patch I've been dabbling with some changes of my own to demonstrate a possible direction.</div><div> </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><br></div><div>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><br>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><br>The only thing missing to make DWARFUnitSection not expose begin/end (and thus, not expose any container-like API) is to </div><div> </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><br></div><div>Why is this a problem? Oh, I suppose getUnitForOffset doesn't work so well when it's not a contiguous non-overlapping range?<br><br>Fair enough.<br><br>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><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></blockquote><div><br></div><div>Fair enough - the attached patch just moves it as-is, which is no worse than the current state, I don't think.</div><div>​<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"><img style="vertical-align: bottom; border: none;" src="https://ssl.gstatic.com/docs/doclist/images/icon_10_generic_list.png"> <span dir="ltr" style="color:#15c;text-decoration:none;vertical-align:bottom">DWARFUnitSection.diff</span></a></div>​<br>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><br>(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><br>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> </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>
<br>
<a href="http://reviews.llvm.org/D5369" target="_blank">http://reviews.llvm.org/D5369</a><br>
<br>
<br>
</blockquote></div><br></div></div>