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

[Pedro Alves <palves at redhat dot com>]

On 12/18/2013 10:06 PM, Tom Tromey wrote:
> Pedro> Yeah, not sure either...  squash a few; post only the non-trivial
> Pedro> ones as email; post all of them at once anyway; split in 4 or
> Pedro> 5 batches.  Whatever really is fine with me.
> 
> In case you want to see my progress, I've pushed the current branch to
> my gitorious repository:
> 
> git@gitorious.org:binutils-gdb/gdb.git

I guess that only works for you.  I used:

  git://gitorious.org/binutils-gdb/gdb.git

I like what I see a lot.

> 
> The branch is "target-cleanup".
> 
> I've built the branch but barely tested it.  I'm sure it breaks in some
> configurations -- the fixes are trivial but I did have to do a number of
> them as my refactoring script, while fun, could not perfectly fix
> everything.
> 
> Aside from a few patches at the beginning of the series I think it's all
> split and ordered pretty much as I'd like it to be.
> 
> I converted all the easy target methods to the new inheritance scheme.
> This works out to be nearly all of them.  There are a few more that
> aren't too bad to convert (actually, looking again now I see I missed a
> few more easy ones), and then a handful of tricky ones.
> 
> It's over the 200 patch mark... not sure I want to write those
> ChangeLogs (my refactoring script only did the first 100 or so for
> me...) or even re-read them all for sanity.
> 
> I hope you will agree that the result is much cleaner.  It's simpler to
> explain and understand; and it is obvious which model one ought to
> choose when adding new target methods.  It's also a good setup for the
> multi-target work.

Yes, I definitely agree.

Only thing I wonder is whether:

> /* This is used to indicate the default implementation for a target
>    method.  It is handled by make-target-delegates.  The argument can be:
>    . A simple constant, like "0", in which case the default method
>    returns the constant.
>    . A function call, like "tcomplain ()", in which case the default
>    method simply calls the function.  Ordinarily such functions must
>    therefore be "noreturn".
>    . The name of a function, like "memory_insert_breakpoint", in which
>    case the named function is used as the default method.  */
> #define TARGET_DEFAULT(ARG)

wouldn't be better be made less smart, and require explicit
selection.  I'm thinking of grepability, but mainly, to be
able to distinguish returning enums vs forwarding.  As in, for
example, how do you tell that:

          TARGET_DEFAULT(TARGET_XFER_E_IO)

is not meant to forward to a TARGET_XFER_E_IO target-like
function?  We could look at whether the symbol is uppercase,
but then that seems to be getting fragile and harder
to explain/read/'quickly grasp what happens if you're not
familiar with the target symbols'.

#define TARGET_DEFAULT_FORWARD(function_to_forward_to)
   e.g.:  TARGET_DEFAULT_FORWARD(memory_insert_breakpoint)

#define TARGET_DEFAULT_RET(expression) // constant, function call, etc.
   e.g.:  TARGET_DEFAULT_RET(0)
          TARGET_DEFAULT_RET(TARGET_XFER_E_IO)
          TARGET_DEFAULT_RET(return_zero ())

#define TARGET_DEFAULT_NO_RET(noreturn function/expression)
   e.g.: TARGET_DEFAULT_NO_RET(noprocess ())

(I'm hating the idea of such a change causing a bunch of
redo-series pain, so even if it proves better to be explicit,
I'm fine with leaving it as is for now.)

-- 
Pedro Alves