[commit] cleanup stale exec.{h|c} xfer_memory comments.

[commit] cleanup stale exec.{h|c} xfer_memory comments.

[Pedro Alves]

The comment describing section_table_xfer_memory_partial is actually
still describing the old xfer_memory.  This removes that stale
description, and adjusts the description in the header a bit better
to current reality.

Checked in.

-- 
Pedro Alves

2009-06-12  Pedro Alves  <pedro@codesourcery.com>

	* exec.h (section_table_xfer_memory_partial): Improve description,
	mention SECTION_NAME.
	* exec.c (section_table_xfer_memory_partial): Remove stale
	description.

---
 gdb/exec.c |   21 ---------------------
 gdb/exec.h |   10 +++++++---
 2 files changed, 7 insertions(+), 24 deletions(-)

Index: src/gdb/exec.c
===================================================================
--- src.orig/gdb/exec.c	2009-06-12 19:18:37.000000000 +0100
+++ src/gdb/exec.c	2009-06-12 19:34:48.000000000 +0100
@@ -559,26 +559,6 @@ map_vmap (bfd *abfd, bfd *arch)
   return vp;
 }
 \f
-/* Read or write from BFD executable files.
-
-   MEMADDR is an address within the target address space, MYADDR is an
-   address within GDB address-space where data is written to, LEN is
-   length of buffer, and WRITE indicates whether to read or write.
-   SECTIONS and SECTIONS_END defines a section table holding sections
-   from possibly multiple BFDs.
-
-   If SECTION_NAME is not NULL, only access sections with that same
-   name.
-
-   Result is a length:
-
-   0:    We cannot handle this address and length.
-   > 0:  We have handled N bytes starting at this address.
-   (If N == length, we did it all.)  We might be able
-   to handle more bytes beyond this length, but no
-   promises.
-   < 0:  We cannot handle this address, but if somebody
-   else handles (-N) bytes, we can start from there.  */
 
 int
 section_table_xfer_memory_partial (gdb_byte *readbuf, const gdb_byte *writebuf,
@@ -595,7 +575,6 @@ section_table_xfer_memory_partial (gdb_b
   if (len <= 0)
     internal_error (__FILE__, __LINE__, _("failed internal consistency check"));
 
-
   for (p = sections; p < sections_end; p++)
     {
       if (section_name && strcmp (section_name, p->the_bfd_section->name) != 0)
Index: src/gdb/exec.h
===================================================================
--- src.orig/gdb/exec.h	2009-06-12 19:31:57.000000000 +0100
+++ src/gdb/exec.h	2009-06-12 19:33:11.000000000 +0100
@@ -39,12 +39,16 @@ extern int build_section_table (struct b
 
 extern int resize_section_table (struct target_section_table *, int);
 
-/* Request to transfer up to LEN 8-bit bytes of the target sections
+/* Read or write from mappable sections of BFD executable files.
+
+   Request to transfer up to LEN 8-bit bytes of the target sections
    defined by SECTIONS and SECTIONS_END.  The OFFSET specifies the
    starting address.
+   If SECTION_NAME is not NULL, only access sections with that same
+   name.
 
-   Return the number of bytes actually transfered, or non-positive
-   when no data is available for the requested range.
+   Return the number of bytes actually transfered, or zero when no
+   data is available for the requested range.
 
    This function is intended to be used from target_xfer_partial
    implementations.  See target_read and target_write for more

Re: [commit] cleanup stale exec.{h|c} xfer_memory comments.

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Fri, 12 Jun 2009 19:43:07 +0100
> 
> The comment describing section_table_xfer_memory_partial is actually
> still describing the old xfer_memory.  This removes that stale
> description, and adjusts the description in the header a bit better
> to current reality.

Is the convention to describe functions in headers?  That's reasonable
for data structures, but we have a lot of functions documented right
before their source, not in the headers.  I find the documentation in
the .c files easier to use, because you don't need to consult another
file.  This is C, not C++, so the interface and the implementation are
not separated.

Re: [commit] cleanup stale exec.{h|c} xfer_memory comments.

[Joel Brobecker]

> Is the convention to describe functions in headers?  That's reasonable
> for data structures, but we have a lot of functions documented right
> before their source, not in the headers.  I find the documentation in
> the .c files easier to use, because you don't need to consult another
> file.  This is C, not C++, so the interface and the implementation are
> not separated.

Unfortunately, I don't think we really have a hard convention in GDB.
For C, I also tend to prefer documenting the function next to the
implementation. It's the only way to be consistent, since some functions
do not have advance declarations.

-- 
Joel

Re: [commit] cleanup stale exec.{h|c} xfer_memory comments.

[Pedro Alves]

On Saturday 13 June 2009 04:47:05, Eli Zaretskii wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Fri, 12 Jun 2009 19:43:07 +0100
> > 
> > The comment describing section_table_xfer_memory_partial is actually
> > still describing the old xfer_memory.  This removes that stale
> > description, and adjusts the description in the header a bit better
> > to current reality.
> 
> Is the convention to describe functions in headers?

I don't believe there's a convention here.  Some modules
describe exported functions in the .c file, others in the .h
file, others in both.  E.g., just looking around, I see gdbthread.h,
inferior.h, serial.h, charset.h, dwarf2-frame.h, describing functions
in the headers.  The other exported functions of exec.c were
described in the header (and the ones that are new came from
target.h, that describes many function in the header too, including
the ones related to section_table_xfer_memory_partial).  I thought
I'd follow suit for consistency with the surrounding code.

> That's reasonable 
> for data structures, but we have a lot of functions documented right
> before their source, not in the headers.  I find the documentation in
> the .c files easier to use, because you don't need to consult another
> file.  

Fine with me.  But if we're going to move the description of
this function, we should move all the others in exec.h too.  Shall
I do this, or do you want to do it?

> This is C, not C++, so the interface and the implementation are 
> not separated.

I must not understand header files then.  (I've no idea why C++
is being referenced here.)

-- 
Pedro Alves

Re: [commit] cleanup stale exec.{h|c} xfer_memory comments.

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Sat, 13 Jun 2009 12:50:02 +0100
> Cc: gdb-patches@sourceware.org
> 
> The other exported functions of exec.c were
> described in the header (and the ones that are new came from
> target.h, that describes many function in the header too, including
> the ones related to section_table_xfer_memory_partial).  I thought
> I'd follow suit for consistency with the surrounding code.

I didn't know that exec.c describes functions in the headers.  I'm
traveling and have no easy access to GDB sources.  I just saw that you
removed the description from exec.c and added the more accurate one to
exec.h.

> > That's reasonable 
> > for data structures, but we have a lot of functions documented right
> > before their source, not in the headers.  I find the documentation in
> > the .c files easier to use, because you don't need to consult another
> > file.  
> 
> Fine with me.  But if we're going to move the description of
> this function, we should move all the others in exec.h too.  Shall
> I do this, or do you want to do it?

I will be able to do that only  in a few days, when I return home.  So
if you have time before that, please do it.  Or maybe we should wait
for others to chime in: this is a matter of personal preferences, and
I've been burned before asking to adhere to mine.

> > This is C, not C++, so the interface and the implementation are 
> > not separated.
> 
> I must not understand header files then.  (I've no idea why C++
> is being referenced here.)

Forget it.

Re: [commit] cleanup stale exec.{h|c} xfer_memory comments.

[Pedro Alves]

On Saturday 13 June 2009 13:24:48, Eli Zaretskii wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Sat, 13 Jun 2009 12:50:02 +0100
> > Cc: gdb-patches@sourceware.org
> > 
> > The other exported functions of exec.c were
> > described in the header (and the ones that are new came from
> > target.h, that describes many function in the header too, including
> > the ones related to section_table_xfer_memory_partial).  I thought
> > I'd follow suit for consistency with the surrounding code.
> 
> I didn't know that exec.c describes functions in the headers.  I'm
> traveling and have no easy access to GDB sources.  I just saw that you
> removed the description from exec.c and added the more accurate one to
> exec.h.

Close, I adjusted the one that was already there in exec.h.  True
enough, I added the original description in exec.h recently ...

> > Fine with me.  But if we're going to move the description of
> > this function, we should move all the others in exec.h too.  Shall
> > I do this, or do you want to do it?
> 
> I will be able to do that only  in a few days, when I return home.  So
> if you have time before that, please do it.  Or maybe we should wait
> for others to chime in: this is a matter of personal preferences, and
> I've been burned before asking to adhere to mine.

For modules that are isolated and used by several other components,
(e.g., gdbthread.h, breakpoints.h), as an API client, I prefer to have
functions described in the headers.  But this case doesn't qualify
much as such, so ... find below the patch that moves the comments
around.

Joel already said he prefers descriptions in .c files, so if
nobody objects, or if I hear a "go ahead", I'll commit the below
with a changelog entry, and be done with it.

-- 
Pedro Alves


---
 gdb/exec.c |   27 +++++++++++++++++++++++++--
 gdb/exec.h |   35 -----------------------------------
 2 files changed, 25 insertions(+), 37 deletions(-)

Index: src/gdb/exec.c
===================================================================
--- src.orig/gdb/exec.c	2009-06-13 14:16:34.000000000 +0100
+++ src/gdb/exec.c	2009-06-13 14:19:03.000000000 +0100
@@ -397,6 +397,9 @@ add_to_section_table (bfd *abfd, struct 
   (*table_pp)++;
 }
 
+/* Resize the section table held by TABLE, by NUM_ADDED.  Returns the
+   old size.  */
+
 int
 resize_section_table (struct target_section_table *table, int num_added)
 {
@@ -559,6 +562,22 @@ map_vmap (bfd *abfd, bfd *arch)
   return vp;
 }
 \f
+/* Read or write from mappable sections of BFD executable files.
+
+   Request to transfer up to LEN 8-bit bytes of the target sections
+   defined by SECTIONS and SECTIONS_END.  The OFFSET specifies the
+   starting address.
+   If SECTION_NAME is not NULL, only access sections with that same
+   name.
+
+   Return the number of bytes actually transfered, or zero when no
+   data is available for the requested range.
+
+   This function is intended to be used from target_xfer_partial
+   implementations.  See target_read and target_write for more
+   information.
+
+   One, and only one, of readbuf or writebuf must be non-NULL.  */
 
 int
 section_table_xfer_memory_partial (gdb_byte *readbuf, const gdb_byte *writebuf,
@@ -644,8 +663,12 @@ exec_xfer_partial (struct target_ops *op
 }
 \f
 
+/* Prints info about all sections defined in the TABLE.  ABFD is
+   special cased --- it's filename is omitted; if it is the executable
+   file, its entry point is printed.  */
+
 void
-print_section_info (struct target_section_table *t, bfd *abfd)
+print_section_info (struct target_section_table *table, bfd *abfd)
 {
   struct target_section *p;
   /* FIXME: 16 is not wide enough when gdbarch_addr_bit > 64.  */
@@ -657,7 +680,7 @@ print_section_info (struct target_sectio
   if (abfd == exec_bfd)
     printf_filtered (_("\tEntry point: %s\n"),
                      paddress (bfd_get_start_address (abfd)));
-  for (p = t->sections; p < t->sections_end; p++)
+  for (p = table->sections; p < table->sections_end; p++)
     {
       printf_filtered ("\t%s", hex_string_custom (p->addr, wid));
       printf_filtered (" - %s", hex_string_custom (p->endaddr, wid));
Index: src/gdb/exec.h
===================================================================
--- src.orig/gdb/exec.h	2009-06-13 14:16:32.000000000 +0100
+++ src/gdb/exec.h	2009-06-13 14:19:17.000000000 +0100
@@ -28,59 +28,24 @@ struct bfd;
 
 extern struct target_ops exec_ops;
 
-/* Builds a section table, given args BFD, SECTABLE_PTR, SECEND_PTR.
-   Returns 0 if OK, 1 on error.  */
-
 extern int build_section_table (struct bfd *, struct target_section **,
 				struct target_section **);
 
-/* Resize the section table held by TABLE, by NUM_ADDED.  Returns the
-   old size.  */
-
 extern int resize_section_table (struct target_section_table *, int);
 
-/* Read or write from mappable sections of BFD executable files.
-
-   Request to transfer up to LEN 8-bit bytes of the target sections
-   defined by SECTIONS and SECTIONS_END.  The OFFSET specifies the
-   starting address.
-   If SECTION_NAME is not NULL, only access sections with that same
-   name.
-
-   Return the number of bytes actually transfered, or zero when no
-   data is available for the requested range.
-
-   This function is intended to be used from target_xfer_partial
-   implementations.  See target_read and target_write for more
-   information.
-
-   One, and only one, of readbuf or writebuf must be non-NULL.  */
-
 extern int section_table_xfer_memory_partial (gdb_byte *, const gdb_byte *,
 					      ULONGEST, LONGEST,
 					      struct target_section *,
 					      struct target_section *,
 					      const char *);
-
-/* Set the loaded address of a section.  */
 extern void exec_set_section_address (const char *, int, CORE_ADDR);
 
-/* Remove all target sections taken from ABFD.  */
-
 extern void remove_target_sections (bfd *abfd);
 
-/* Add the sections array defined by [SECTIONS..SECTIONS_END[ to the
-   current set of target sections.  */
-
 extern void add_target_sections (struct target_section *sections,
 				 struct target_section *sections_end);
 
-/* Prints info about all sections defined in the TABLE.  ABFD is
-   special cased --- it's filename is omitted; if it is the executable
-   file, its entry point is printed.  */
-
 extern void print_section_info (struct target_section_table *table,
 				bfd *abfd);
 
-
 #endif

Re: [commit] cleanup stale exec.{h|c} xfer_memory comments.

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Sat, 13 Jun 2009 14:28:04 +0100
> Cc: gdb-patches@sourceware.org
> 
> find below the patch that moves the comments around.

Thanks!

Re: [commit] cleanup stale exec.{h|c} xfer_memory comments.

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Unfortunately, I don't think we really have a hard convention in GDB.
Joel> For C, I also tend to prefer documenting the function next to the
Joel> implementation. It's the only way to be consistent, since some functions
Joel> do not have advance declarations.

FWIW, I prefer to have documentation in the header for a module's
public API, and next to the implementation for the private API.
Consistency doesn't matter as much to me as being able to read a
header file and get a grasp of how I would use a module; the private
comments in the module can then describe the implementation.

There are several examples of this in gdb already.  Consider:
macro*.[ch], bcache.[ch], dictionary.[ch], addrmap.[ch].

These are all exemplary code, which I think is not coincidental...  it
requires more work to write the code this way, but that forces one to
consciously consider the API.

Tom

Re: [commit] cleanup stale exec.{h|c} xfer_memory comments.

[Joel Brobecker]

> FWIW, I prefer to have documentation in the header for a module's
> public API, and next to the implementation for the private API.
> Consistency doesn't matter as much to me as being able to read a
> header file and get a grasp of how I would use a module; the private
> comments in the module can then describe the implementation.

I would be OK with this approach too.

-- 
Joel

Re: [commit] cleanup stale exec.{h|c} xfer_memory comments.

[Stan Shebs]

Tom Tromey wrote:
>>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
>>>>>>             
>
> Joel> Unfortunately, I don't think we really have a hard convention in GDB.
> Joel> For C, I also tend to prefer documenting the function next to the
> Joel> implementation. It's the only way to be consistent, since some functions
> Joel> do not have advance declarations.
>
> FWIW, I prefer to have documentation in the header for a module's
> public API, and next to the implementation for the private API.
> Consistency doesn't matter as much to me as being able to read a
> header file and get a grasp of how I would use a module; the private
> comments in the module can then describe the implementation.
>   
I think that in many cases functions in a header don't get documentation 
there because they are intended to be semi-private, and are only in a 
header because of the rules of C and our own conventions.  For such 
functions it would at least be useful to have a line "semi-private, 
don't assume you can use this for your own purposes".

Should we maybe introduce a coding rule requiring at least a brief 
API/usage comment about each function declaration in a header? Perhaps 
all the semi-private functions can be separated into a block with a 
comment that applies to the lot of them.

Stan

Re: [commit] cleanup stale exec.{h|c} xfer_memory comments.

[Tom Tromey]

>>>>> "Stan" == Stan Shebs <stan@codesourcery.com> writes:

Stan> I think that in many cases functions in a header don't get
Stan> documentation there because they are intended to be semi-private, and
Stan> are only in a header because of the rules of C and our own
Stan> conventions.  For such functions it would at least be useful to have a
Stan> line "semi-private, don't assume you can use this for your own
Stan> purposes".

Yeah.  My ideal in these cases is to have a second header which is
private to the implementation.

Stan> Should we maybe introduce a coding rule requiring at least a brief
Stan> API/usage comment about each function declaration in a header? Perhaps
Stan> all the semi-private functions can be separated into a block with a
Stan> comment that applies to the lot of them.

It would be fine by me, for public APIs.  For existing messy headers,
I don't care so much (unless someone wants to do some big cleanups on
them), but I would like it if new headers were to be written to a
Blandyesque standard.

Tom