<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 17, 2014 at 4:19 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Wed, Sep 17, 2014 at 3:43 PM, Alexey Samsonov <span dir="ltr"><<a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@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"><div class="gmail_extra"><br><div class="gmail_quote"><div><div>On Tue, Sep 16, 2014 at 11:11 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>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></span><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><span><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></span><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><span><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></span><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><span><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></span><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;min-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" style="display:inline-block;max-width:366px;overflow:hidden;text-overflow:ellipsis;white-space:nowrap;text-decoration:none;padding:1px 0;border:none" target="_blank"><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></div></blockquote><div><br></div></div></div><div>I think I just referred to inheritance vs. composition. IMO it's better to have SmallVector<DWARFUnit> as a member in DWARFUnitSection. The latter will certainly serve as "container" for its users, that's fine.</div><div>llvm::Module contains a list of functions, not inherits from it, and defines its own begin()/end() methods. I think we should do the same here.</div></div></div></div></blockquote><div><br></div></div></div><div>Right - I wasn't suggesting (especially public) inheritance was the solution here, I was just (/slightly/) arguing in favor of exposing begin/end wrappers around the (composed) container member, rather than "units()" returning a range. A minor point.<br></div></div></div></div></blockquote><div><br></div><div>Sure, I'm fine with either of this.</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></div><span class=""><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"><span><div><br></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"><span><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></span></div><br></div></div>
</blockquote></span></div><span><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div dir="ltr">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div>
</font></span></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div>
</div></div>