Re: [PATCH v4 3/9] add target method delegation

[Pedro Alves <palves at redhat dot com>]

On 12/13/2013 10:07 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:
> 
> Tom> Since this is very repetitive I would consider doing it via a ".defs"
> Tom> file and then some macrology to reduce the amount of boilerplate.
> 
> I tried this a bit and the result was pretty ugly.
> 
> Instead I wrote a little script that can parse just enough of "struct
> target_ops" to auto-generate delegation methods when appropriate.
> 
> See patch #1 attached below.  This patch just converts the methods used
> by the record targets -- that is, it's just a replacement for patch #3
> in this series.
> 
> Note that the patch isn't ready as-is.  The log entry is wrong and it
> does the wrong thing for a couple of the target methods.  I'm curious to
> know what you think about the overall shape of it.

I like it a lot.

> 
> 
> As an example of the benefit of this approach, patch #2 converts
> to_detach to the new style.  When examining this patch recall that
> target-delegates.c is auto-generated.  You can see from this that the
> result is less code to maintain.
> 
> 
> I then went a little further.  It seems to me that we also need a lot of
> boilerplate (not to mention the questionable casts there right now) to
> handle the dummy target methods.  Patch #3 makes these into simple
> annotations in the target.h file, getting rid of the list of handled
> methods from make-target-delegates as well.

Nice.

> 
> 
> Before reaching judgment on patch #3, see patch #4, which converts
> to_attach.  I've written a few more like it.
> 
> 
> The long-term vision for this approach is to regularize all target
> methods according to these rules:
> 
> 1. Each method will take a "struct target_ops *" as its first argument.
> 
> 2. Nearly every method will be defaulted to the appropriate delegate_
>    method.  There may be exceptions, coded into
>    complete_target_initialization.
> 
> 3. de_fault will be completely eliminated.  INHERIT will be limited to
>    the smallest possible list of fields, like to_shortname.  Nearly
>    every current_target method will simply delegate.

Sounds like the squashed target will end up mostly unused?  It sounds
to me that even for those (data fields) we wouldn't need INHERIT.  As
all targets have a name, e.g., target_shortname could be made a method
that returns the name of the currently topmost target in the stack
(not the squashed target).

> 
> 4. Target API users will call top-level target_* functions that will in
>    most cases just call into current_target.
> 
> 5. Any given to_* method implementation will be guaranteed to be called
>    with the target_ops with which it was registered.  (This rule is
>    handy on multi-target, where I split out the target_ops vtable from
>    the object itself.)
> 
> 
> Since I'm on a cleanup kick at the moment, and since a good amount of
> this is relevant to the multi-target work, I think it would be good to
> start the conversion shortly after 7.7 is branched.
> 
> This is a bit difficult since I don't have access to many targets.  I'm
> not sure what to do about this aspect.

What I've done in that past for this sort of thing is just go ahead
and do the almost mechanical adjustment across the board, and don't
fret about it if something breaks.  Publish git test branches, IOW,
make it very easy for people to test, report issues and hopefully
help with targets they care about should help drive this through
faster.

-- 
Pedro Alves