Re: [patch/rfc] Simplify target stack

[Daniel Jacobowitz <drow@mvista.com>]

On Thu, Oct 16, 2003 at 11:27:52AM -0400, Andrew Cagney wrote:
> >On Wed, Oct 15, 2003 at 06:37:46PM -0400, Andrew Cagney wrote:
> >
> >>Hello,
> >>
> >>The attached patch simplifies the target-stack by folding the "struct 
> >>target_stack_item" into the "struct target_ops".  The field "struct 
> >>target_ops . beneath" being added.
> >>
> >>This in turn greatly simplifies the logic needed to walk the target 
> >>stack (target_beneath becomes a one-liner), and that in turn lets me 
> >>correctly implement the new target read/write partial methods I just 
> >>posted.
> >>
> >>Note that this implementation is still limited to a single target stack 
> >>(due to all the target_ops vectors being static).  Follow-on changes can 
> >>eliminate that restriction.
> >>
> >>Once I've finished testing, I'll look to commit it in a day or so,
> >>Andrew
> >
> >
> >You're moving beneath into target_ops, but aren't you going to have to
> >either move it out again or move everything else from target_ops?  It
> >seems to me that we want the method vector to be constant eventually
> >(kill the INHERIT mess), but the target to have local data.  Just seems
> >like this is happening in the wrong order.  Another way would be:
> >  - rename target_ops and target_item
> >  - make access to target go through the renamed version of target_item
> >  - add a target_data member to the renamed target_data
> 
> I believe that the objectives here are:
> 
> 1. being able to directly walk the target chain
> Makes it possible to eliminate the INHERIT mess.  Lets targets 
> efficiently/directly interact with target-beneath.
> 
> 2. allow multiple instances of a specific target
> So that more than one target stack is possible.
> 
> 3. strict separation of target instance and target ops
> See below.
> 
> In terms of priority, I rank them as above.

But we already _have_ a separation of target instance and target ops. 
It's struct target_stack_item.  It's your cleanup right there, waiting
to happen.  Removing it is not a step forwards; you can just change to
passing that item around instead of struct target_ops.  If you want a
different name, rename it.  Not everywhere will need to be converted,
obviously - only things which want the new data.

> My immediate objective is the first one - make the target stack directly 
> accessible.  That way I can implement target read/write partial in a 
> relatively clean way (the INHERIT in the current patch can go).  It also 
> comes with significant bang for the buck - it opens the way for an 
> incremental cleanup of the target vectors.
> 
> The second objective has to be considered medium term.  Any direct 
> references to static variables, and the currently static target ops will 
> need to go.  Fortunatly, it can be done on an as-needed basis.
> 
> This change can be implemented out without worrying about the third 
> objective - just clone the existing registered target vector when a new 
> instance is required.
> 
> For the last one, some background is needed.  The original target code 
> implemented the target ops using completly static structures 
> Unfortunatly this proved to be un-maintainable - adding and removing 
> fields broke things - and was replaced by runtime initialization (this 
> is also one of the reasons behind the architecture vector using run-time 
> initialization).   While the code migh eventually be massaged into 
> run-time initializing a separate "struct target_ops", I don't think its 
> exactly urgent.

I'm not sure what you mean by run-time initialization in this context,
since we already have run-time initializers for struct target_ops -
they initialize the static structures right now.  Almost (not quite,
but pretty close) to everything in that initialized structure is ops. 
Eventually, with low urgency, the non-ops should be moved out of it. 
But for now that's no excuse to move things into it that you will have
to move out of it again.

Just because you can avoid doing it now doesn't mean that's OK.  You
tell that to other developers at every opportunity.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer