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: Doug Evans <dje at google dot com>
- To: Gary Benson <gbenson at redhat dot com>
- Cc: gdb-patches <gdb-patches at sourceware dot org>, Pedro Alves <palves at redhat dot com>
- Date: Thu, 11 Sep 2014 10:12:03 -0700
- 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>
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_. :-)