[PATCH] Move DWARFUnitSection from the vector API to something more semantically accurate.

[Frédéric Riss]

> On 17 Sep 2014, at 08:11, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Sep 16, 2014 at 10:47 PM, Frederic Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
> 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.
> 
> Yeah, I saw from his CR feedback.
> 
> To be honest, since seeing this patch I've been dabbling with some changes of my own to demonstrate a possible direction.
>  
> The next step for me is to factor the the parsing code into this 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. 
> 
> 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).
> 
> The only thing missing to make DWARFUnitSection not expose begin/end (and thus, not expose any container-like API) is to 
>  
> 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.
> 
> Why is this a problem? Oh, I suppose getUnitForOffset doesn't work so well when it's not a contiguous non-overlapping range?
> 
> Fair enough.
> 
> 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)

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? :-))

>  
> Once this is fixed, we can move parsing into DWARFUnitSection and remove at least the addUnit method.
> 
> Fair enough - the attached patch just moves it as-is, which is no worse than the current state, I don't think.
> ​
>  DWARFUnitSection.diff <https://docs.google.com/file/d/0B0jpkch3iC_7ZXRwR1d5cGFOcVk/edit?usp=drive_web>
I don’t have permission to access that file. I guess it’s pretty similar to what my topic branch looks like.

> ​
> 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.

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.

Fred

> (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)
> 
> 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.
>  
> Once this is done a bunch of accessors from the context can go away also.
> 
> http://reviews.llvm.org/D5369 <http://reviews.llvm.org/D5369>
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140917/3aa4ebc8/attachment.html>