This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 5/9 v7] Introduce common-regcache.h
- From: Gary Benson <gbenson at redhat dot com>
- To: Doug Evans <dje at google dot com>
- Cc: gdb-patches <gdb-patches at sourceware dot org>, Pedro Alves <palves at redhat dot com>
- Date: Fri, 12 Sep 2014 10:45:26 +0100
- Subject: Re: [PATCH 5/9 v7] Introduce common-regcache.h
- Authentication-results: sourceware.org; auth=none
- References: <1409320299-6812-1-git-send-email-gbenson at redhat dot com> <1409320299-6812-6-git-send-email-gbenson at redhat dot com> <21520 dot 37315 dot 16694 dot 677898 at ruffy2 dot mtv dot corp dot google dot com> <20140911110231 dot GC17472 at blade dot nx> <CADPb22SvNvFAXoSQ_HkyeWTw2TfYPmesamsAfDO-tjdH4Mjx1A at mail dot gmail dot 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/