Re: [PATCH 5/9 v7] Introduce common-regcache.h

[Doug Evans <dje@google.com>]

On Thu, Sep 11, 2014 at 4:02 AM, Gary Benson <gbenson@redhat.com> wrote:
> Doug Evans wrote:
>> Gary Benson writes:
>> > +/* Return the register cache associated with the thread specified
>> > +   by PTID.  This function must be provided by the client.  */
>>
>> Can you add a comment here explaining the ownership of the memory
>> object returned?  E.g., is it cached "internally" to the function
>> so that the caller doesn't have to free it?
>
> That seems odd.  We don't document other similar functions this way:
> I'm thinking GDB's get_current_arch, current_inferior, target_gdbarch,
> or gdbserver's current_process, current_target_desc.  It seems the
> pattern is to note if the caller must free the object and to remain
> quiet otherwise.

Yeah, but I never liked such things being implicit.
I can't trust a missing "caller must free" as being intentional.
[One can equally argue the presence of "caller must free" (or "not
free") isn't necessarily trustable, but such things don't change that
often.]

With a name like "get_current_foo", the "current" makes things less
implicit (at least to me).

If we were using c++ then object ownership can (often, though not
always) be more clearly expressed and then such things can be more
reasonably left as being implicit in comments.
But we don't have that so if we're going to be cleaning things up, and
maybe even paying a little attention to API design, I figure IWBN to
have things be clearer than they are today.

[Plus I get bitten time and again by taking gdb's existing practice as
something we actually want to keep. :-)]

> How about I change the comment to "Return _a_pointer_to_ the register
> cache..."?  (underlines for emphasis here).

If one was going to add emphasis, I'd emphasize _the_. :-)