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

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Could you please split out the patch for the second problem?  I
Pedro> think it'll be a small one.

Sure.

Pedro> What's the plan for the existing target methods that
Pedro> currently already do a similar beneath lookup?  I'd like it that
Pedro> there's be at least a plan, so we don't end up with yet another
Pedro> way of doing things, two incomplete transitions, and no clear direction.

Do you mean things like target_detach?

If so, then I think these are two different things.

target.h declares things both for users of the target API and for the
implementation of targets.

Something like target_detach is a public API.  My understanding of the
current scheme is that a public-facing method can either be implemented
by a function like target_detach, which encodes all the needed logic; or
by a macro that just calls into the target_ops, in which case the method
must use the inherit/default machinery.

On the other hand, target_delegate_* functions are there for the target
implementations to use.  They just encapsulate a bit of common code for
the case where a target wants to delegate a request to the next lower
target on the stack.

>> + gdb_assert_not_reached (_("reached end of target stack during
>> delegation"));
>> +}

Pedro> This appears in several places.  Whenever I see the same string
Pedro> repeated over and over, I tend to think it'd be good to add a
Pedro> utility helper function:

Will do.

Pedro> Some of the delegation methods have that assert, while others don't
Pedro> have anything at the tail end.  What's the story there?

I don't remember.  It does seem a bit random right now.

It seems to me that the best approach is to make a target_delegate_*
function assert only when there is no de_fault for the corresponding
method.

>> +      if (t->to_insert_breakpoint)

Pedro>     if (t->to_insert_breakpoint != NULL)

I fixed all the instances of it.

Tom