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

[Gary Benson <gbenson@redhat.com>]

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

-- 
http://gbenson.net/