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

Gary Benson
2014-09-12T09:45:32Z

Doug Evans wrote:
> 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_. :-)

I tried to figure out how to succinctly write what you're asking me
to here but I'm stuck. Is there a function documented in this way
in GDB I can use as an example?

In the interests of keeping things moving I've pushed the patch with
this comment:

Return a pointer to the register cache associated with the
thread specified by PTID. This function must be provided by
the client.

Thanks,
Gary

--