[patch] Code cleanup: Make function typedef for find memory region

[patch] Code cleanup: Make function typedef for find memory region

[Jan Kratochvil]

Hi,

This is a code cleanup without any compiled code changes.

Currently
	int (*func) (CORE_ADDR, unsigned long, int, int, int, void *),

is used on many places to find memory regions.  Changing the prototype
requires even changes on places which would not have to be changed otherwise.

I see I have chosen the *_t type name again, to be decided upon replies to:
	http://sourceware.org/ml/gdb-patches/2010-08/msg00497.html

This is only a prerequisite for the next PR 11804 fix.

IIRC I have compiled it on OpenSolaris (procfs.c).

No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu.


Thanks,
Jan


gdb/
2010-08-30  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Code cleanup.
	* defs.h (find_memory_region_t): New typedef.
	(exec_set_find_memory_regions): Use it.
	* exec.c (exec_set_find_memory_regions): Use find_memory_region_t.
	* fbsd-nat.c (fbsd_find_memory_regions): Likewise.
	* gcore.c (objfile_find_memory_regions): Likewise.
	* gnu-nat.c (gnu_find_memory_regions): Likewise.
	* linux-nat.c (linux_nat_find_memory_regions): Likewise.
	* procfs.c (iterate_over_mappings_cb_ftype): Remove.
	(iterate_over_mappings): Rename iterate_over_mappings_cb_ftype to
	find_memory_region_t.
	(insert_dbx_link_bpt_in_region): Likewise.
	(iterate_over_mappings): Likewise.  Drop the comment part about the
	function prototype.
	(find_memory_regions_callback): Use find_memory_region_t.
	(proc_find_memory_regions): Likewise.
	(info_mappings_callback): Rename iterate_over_mappings_cb_ftype to
	find_memory_region_t.
	* target.c (dummy_find_memory_regions): Use find_memory_region_t.
	* target.h (struct target_ops) <to_find_memory_regions>: Likewise.

--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -634,12 +634,13 @@ extern void init_source_path (void);
 
 /* From exec.c */
 
+typedef int (*find_memory_region_t) (CORE_ADDR addr, unsigned long size,
+				     int read, int write, int exec,
+				     void *data);
+
 /* Take over the 'find_mapped_memory' vector from exec.c. */
-extern void exec_set_find_memory_regions (int (*) (int (*) (CORE_ADDR, 
-							    unsigned long, 
-							    int, int, int, 
-							    void *),
-						   void *));
+extern void exec_set_find_memory_regions (int (*func) (find_memory_region_t,
+						       void *));
 
 /* Possible lvalue types.  Like enum language, this should be in
    value.h, but needs to be here for the same reason. */
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -835,11 +835,7 @@ exec_has_memory (struct target_ops *ops)
 /* Find mapped memory. */
 
 extern void
-exec_set_find_memory_regions (int (*func) (int (*) (CORE_ADDR, 
-						    unsigned long, 
-						    int, int, int, 
-						    void *),
-					   void *))
+exec_set_find_memory_regions (int (*func) (find_memory_region_t, void *))
 {
   exec_ops.to_find_memory_regions = func;
 }
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -92,9 +92,7 @@ fbsd_read_mapping (FILE *mapfile, unsigned long *start, unsigned long *end,
    argument to FUNC.  */
 
 int
-fbsd_find_memory_regions (int (*func) (CORE_ADDR, unsigned long,
-				       int, int, int, void *),
-			  void *obfd)
+fbsd_find_memory_regions (find_memory_region_t func, void *obfd)
 {
   pid_t pid = ptid_get_pid (inferior_ptid);
   char *mapfilename;
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -462,9 +462,7 @@ gcore_create_callback (CORE_ADDR vaddr, unsigned long size,
 }
 
 static int
-objfile_find_memory_regions (int (*func) (CORE_ADDR, unsigned long,
-					  int, int, int, void *),
-			     void *obfd)
+objfile_find_memory_regions (find_memory_region_t func, void *obfd)
 {
   /* Use objfile data to create memory sections.  */
   struct objfile *objfile;
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -2487,11 +2487,7 @@ gnu_xfer_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len, int write,
 
 /* Call FUNC on each memory region in the task.  */
 static int
-gnu_find_memory_regions (int (*func) (CORE_ADDR,
-				      unsigned long,
-				      int, int, int,
-				      void *),
-			 void *data)
+gnu_find_memory_regions (find_memory_region_t func, void *data)
 {
   error_t err;
   task_t task;
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4104,9 +4104,7 @@ read_mapping (FILE *mapfile,
    regions in the inferior for a corefile.  */
 
 static int
-linux_nat_find_memory_regions (int (*func) (CORE_ADDR,
-					    unsigned long,
-					    int, int, int, void *), void *obfd)
+linux_nat_find_memory_regions (find_memory_region_t func, void *obfd)
 {
   int pid = PIDGET (inferior_ptid);
   char mapsfilename[MAXPATHLEN];
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -438,17 +438,9 @@ static void free_syscalls (procinfo *pi);
 static int find_syscall (procinfo *pi, char *name);
 #endif /* DYNAMIC_SYSCALLS */
 
-/* A function type used as a callback back iterate_over_mappings.  */
-typedef int (iterate_over_mappings_cb_ftype)
-  (CORE_ADDR vaddr, unsigned long size, int read, int write, int execute,
-   void *data);
-
 static int iterate_over_mappings
-  (procinfo *pi,
-   iterate_over_mappings_cb_ftype *child_func,
-   void *data,
-   int (*func) (struct prmap *map,
-		iterate_over_mappings_cb_ftype *child_func,
+  (procinfo *pi, find_memory_region_t child_func, void *data,
+   int (*func) (struct prmap *map, find_memory_region_t child_func,
 		void *data));
 
 /* The head of the procinfo list: */
@@ -3784,7 +3776,7 @@ solib_mappings_callback (struct prmap *map, int (*func) (int, CORE_ADDR),
 
 static int
 insert_dbx_link_bpt_in_region (struct prmap *map,
-			       iterate_over_mappings_cb_ftype *child_func,
+			       find_memory_region_t child_func,
 			       void *data)
 {
   procinfo *pi = (procinfo *) data;
@@ -5226,11 +5218,10 @@ procfs_use_watchpoints (struct target_ops *t)
    from the callback function, or zero.  */
 
 static int
-iterate_over_mappings (procinfo *pi,
-		       iterate_over_mappings_cb_ftype *child_func,
+iterate_over_mappings (procinfo *pi, find_memory_region_t child_func,
 		       void *data,
 		       int (*func) (struct prmap *map,
-				    iterate_over_mappings_cb_ftype *child_func,
+				    find_memory_region_t child_func,
 				    void *data))
 {
   char pathname[MAX_PROC_NAME_SIZE];
@@ -5282,22 +5273,11 @@ iterate_over_mappings (procinfo *pi,
 }
 
 /* Implements the to_find_memory_regions method.  Calls an external
-   function for each memory region.  The external function will have
-   the signature:
-
-     int callback (CORE_ADDR vaddr,
-		   unsigned long size,
-		   int read, int write, int execute,
-		   void *data);
-
+   function for each memory region.
    Returns the integer value returned by the callback.  */
 
 static int
-find_memory_regions_callback (struct prmap *map,
-			      int (*func) (CORE_ADDR,
-					   unsigned long,
-					   int, int, int,
-					   void *),
+find_memory_regions_callback (struct prmap *map, find_memory_region_t func,
 			      void *data)
 {
   return (*func) ((CORE_ADDR) map->pr_vaddr,
@@ -5321,11 +5301,7 @@ find_memory_regions_callback (struct prmap *map,
    the callback.  */
 
 static int
-proc_find_memory_regions (int (*func) (CORE_ADDR,
-				       unsigned long,
-				       int, int, int,
-				       void *),
-			  void *data)
+proc_find_memory_regions (find_memory_region_t func, void *data)
 {
   procinfo *pi = find_procinfo_or_die (PIDGET (inferior_ptid), 0);
 
@@ -5364,8 +5340,7 @@ mappingflags (long flags)
    mappings'.  */
 
 static int
-info_mappings_callback (struct prmap *map,
-			iterate_over_mappings_cb_ftype *ignore,
+info_mappings_callback (struct prmap *map, find_memory_region_t ignore,
 			void *unused)
 {
   unsigned int pr_off;
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3005,7 +3005,7 @@ dummy_pid_to_str (struct target_ops *ops, ptid_t ptid)
 
 /* Error-catcher for target_find_memory_regions.  */
 static int
-dummy_find_memory_regions (int (*ignore1) (), void *ignore2)
+dummy_find_memory_regions (find_memory_region_t ignore1, void *ignore2)
 {
   error (_("Command not implemented for this target."));
   return 0;
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -504,11 +504,7 @@ struct target_ops
     int (*to_async_mask) (int);
     int (*to_supports_non_stop) (void);
     /* find_memory_regions support method for gcore */
-    int (*to_find_memory_regions) (int (*) (CORE_ADDR,
-					    unsigned long,
-					    int, int, int,
-					    void *),
-				   void *);
+    int (*to_find_memory_regions) (find_memory_region_t func, void *data);
     /* make_corefile_notes support method for gcore */
     char * (*to_make_corefile_notes) (bfd *, int *);
     /* get_bookmark support method for bookmarks */

Re: [patch] Code cleanup: Make function typedef for find memory region

[Pedro Alves]

On Monday 30 August 2010 09:59:53, Jan Kratochvil wrote:
> Hi,
> 
> This is a code cleanup without any compiled code changes.
> 
> Currently
> 	int (*func) (CORE_ADDR, unsigned long, int, int, int, void *),
> 
> is used on many places to find memory regions.  Changing the prototype
> requires even changes on places which would not have to be changed otherwise.
> 
> I see I have chosen the *_t type name again, 

I suggest you don't use _t for this.  Most of our function typedefs
use the *_ftype prefix, a few _func, and only a couple _fn or _t.

Try something like:

$ grep -rn "_ftype)" * | grep -v ChangeLog | grep typedef | wc -l
155

$ grep -rn "_func)" * | grep -v ChangeLog | grep typedef | wc -l
15

...


So, I'd suggest something like:

 find_memory_regions_callback_ftype
 find_memory_regions_ftype
 find_memory_region_callback_ftype
 find_memory_region_ftype

-- 
Pedro Alves

Re: [patch] Code cleanup: Make function typedef for find memory region

[Joel Brobecker]

> So, I'd suggest something like:
> 
>  find_memory_regions_callback_ftype
>  find_memory_regions_ftype
>  find_memory_region_callback_ftype
>  find_memory_region_ftype

FWIW: I like ftype too.

-- 
Joel

Re: [patch] Code cleanup: Make function typedef for find memory region

[Joel Brobecker]

> Currently
> 	int (*func) (CORE_ADDR, unsigned long, int, int, int, void *),
> 
> is used on many places to find memory regions.  Changing the prototype
> requires even changes on places which would not have to be changed otherwise.

I think we should generalize this idea and use function types whenever
possible. Using a function type, for instance, makes it easier for
someone to find the description of that function.  I had that problem
recently, and it took me a long time to find what I was looking for.

I also believe that we should explicitly name the parameters in the
function type declaration.  I think this is important, because I think
that this practice would have helped prevent an unfortunate regression
(filename/fullname confusion in MI command). We should name the parameters
so that we know which parameter is what.  And that also helps IMO writing
the function description.

-- 
Joel

Re: [patch] Code cleanup: Make function typedef for find memory region

[Jan Kratochvil]

On Mon, 30 Aug 2010 16:14:54 +0200, Joel Brobecker wrote:
> I also believe that we should explicitly name the parameters in the
> function type declaration.

I try to do that whenever I can, I am for making it a part of patch reviews.

When we are at the rules:

I am also for requiring comment to be placed at the function definition and
not at its declaration.  Using tag jumps one will never find the declaration
and I have considered these functions to have no comment (randomly found now
simple_displaced_step_copy_insn, it was a different function I had the problem
with).

I am also for forbidding putting comments there at both places as such way
they get inconsistent soon or they differ etc. (randomly found init_type).


Regards,
Jan

Re: [patch] Code cleanup: Make function typedef for find memory region

[Joel Brobecker]

> I am also for requiring comment to be placed at the function definition and
> not at its declaration.  Using tag jumps one will never find the declaration
> and I have considered these functions to have no comment (randomly found now
> simple_displaced_step_copy_insn, it was a different function I had the problem
> with).

That has been my approach as well, so I'm not the one that needs
convincing.  However, proponents of comments with the definition
also have a good point. When you have a nice public API declared
in a .h file, it's convenient to have the documentation there.
I still think that it's better to be consistent in the location
of the documentation, particularly if the names we choose in the API
are clear enough that it gives us a general idea of what each entity
is about.  We can then read the comment of the functions of interest.

> I am also for forbidding putting comments there at both places as such way
> they get inconsistent soon or they differ etc. (randomly found init_type).

Agree on that as well.

-- 
Joel

Re: [patch] Code cleanup: Make function typedef for find memory region

[Jan Kratochvil]

On Mon, 30 Aug 2010 16:58:15 +0200, Joel Brobecker wrote:
> > I am also for requiring comment to be placed at the function definition and
> > not at its declaration.  Using tag jumps one will never find the declaration
> > and I have considered these functions to have no comment (randomly found now
> > simple_displaced_step_copy_insn, it was a different function I had the problem
> > with).
> 
> That has been my approach as well, so I'm not the one that needs
> convincing.  However, proponents of comments with the definition
                                                        ^^^^^^^^^^->declaration
[ The mail would not make sense otherwise.  ]

> also have a good point. When you have a nice public API declared
> in a .h file, it's convenient to have the documentation there.
> I still think that it's better to be consistent in the location
> of the documentation, particularly if the names we choose in the API
> are clear enough that it gives us a general idea of what each entity
> is about.  We can then read the comment of the functions of interest.

Yes, I understand the reasons of the "wrong" placement but IMHO it is not
worth the pain, one can very easily jump forth and back while reading it.


Thanks,
Jan

Re: [patch] Code cleanup: Make function typedef for find memory region

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I am also for requiring comment to be placed at the function
Jan> definition and not at its declaration.  Using tag jumps one will
Jan> never find the declaration and I have considered these functions to
Jan> have no comment (randomly found now
Jan> simple_displaced_step_copy_insn, it was a different function I had
Jan> the problem with).

I think there are three cases.

One case is the "bcache" case: you have a relatively simple data
structure with a defined public API.  In this case, I find it it
convenient to be able to read the header file to see the entire exported
API, without being distracted by the implementation.  This case is maybe
not as typical as we might like; many data types in gdb are semi-opaque
at best.

The second case is implementations of virtual methods.  Here, the
comment belongs at the point of definition.  I think commenting the
method implementation is actually (mildly) bad, because it means copying
documentation, with the problems that implies.

The last case is the rest of gdb -- semi-opaque data structures, utility
functions, etc.  For these I think using the definition is preferable.

That said, my general rule for hacking on gdb is to just follow whatever
style is in use wherever I am hacking.  If I were writing new code, I
might aspire to the bcache approach; but otherwise I just comment the
definition.

Jan> I am also for forbidding putting comments there at both places as
Jan> such way they get inconsistent soon or they differ etc. (randomly
Jan> found init_type).

I agree.

Tom

Re: [patch] Code cleanup: Make function typedef for find memory region

[Jan Kratochvil]

On Mon, 30 Aug 2010 20:38:06 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> I am also for requiring comment to be placed at the function
> Jan> definition and not at its declaration.  Using tag jumps one will
> Jan> never find the declaration and I have considered these functions to
> Jan> have no comment (randomly found now
> Jan> simple_displaced_step_copy_insn, it was a different function I had
> Jan> the problem with).
> 
> I think there are three cases.
> 
> One case is the "bcache" case: you have a relatively simple data
> structure with a defined public API.  In this case, I find it it
> convenient to be able to read the header file to see the entire exported
> API, without being distracted by the implementation.  This case is maybe
> not as typical as we might like; many data types in gdb are semi-opaque
> at best.

This is definitely a disagreement.  Such general guide should be in
doc/gdbint.texinfo .


> The second case is implementations of virtual methods.  Here, the
> comment belongs at the point of definition.  I think commenting the
> method implementation is actually (mildly) bad, because it means copying
> documentation, with the problems that implies.

Implementation should name the field in the interface to be able to jump
there.


> That said, my general rule for hacking on gdb is to just follow whatever
> style is in use wherever I am hacking.

I also do so but I still find it wrong.



Thanks,
Jan

Re: [patch] Code cleanup: Make function typedef for find memory region

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Tom> One case is the "bcache" case: you have a relatively simple data
Tom> structure with a defined public API.  In this case, I find it it
Tom> convenient to be able to read the header file to see the entire exported
Tom> API, without being distracted by the implementation.  This case is maybe
Tom> not as typical as we might like; many data types in gdb are semi-opaque
Tom> at best.

Jan> This is definitely a disagreement.

Yeah.  And of course, I will follow whatever we collectively agree upon.

Jan> Such general guide should be in doc/gdbint.texinfo .

I would rather get rid of this document, if possible.  Having it
separate from the source means that modifying it is usually an
afterthought.  It is better, IMO, to have the comments near the source,
because that increases the likelihood that a reviewer will remember to
ask for comment updates when a patch changes something.

Tom> The second case is implementations of virtual methods.

Jan> Implementation should name the field in the interface to be able to jump
Jan> there.

I like this idea.

Tom

Re: [patch] Code cleanup: Make function typedef for find memory region

[Joel Brobecker]

> Jan> Implementation should name the field in the interface to be able to jump
> Jan> there.
> 
> I like this idea.

The documentation I write in that case is:

    /* Implement the to_do_this target_ops method.  */

This avoid duplication, and refers back to the routine that this
function is supposed to implement.

-- 
Joel

Re: [patch] Code cleanup: Make function typedef for find memory region

[Jan Kratochvil]

On Mon, 30 Aug 2010 12:43:55 +0200, Pedro Alves wrote:
> I suggest you don't use _t for this.  Most of our function typedefs
> use the *_ftype prefix, a few _func, and only a couple _fn or _t.
[...]
>  find_memory_region_ftype

Chose this one.

Otherwise I find it obvious, I will check it in upon approval of:
	[patch] Fix gcore writer for -Wl,-z,relro (PR corefiles/11804)
	http://sourceware.org/ml/gdb-patches/2010-08/msg00499.html


Thanks,
Jan


gdb/
2010-08-31  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Code cleanup.
	* defs.h (find_memory_region_ftype): New typedef.
	(exec_set_find_memory_regions): Use it.
	* exec.c (exec_set_find_memory_regions): Use find_memory_region_ftype.
	* fbsd-nat.c (fbsd_find_memory_regions): Likewise.
	* gcore.c (objfile_find_memory_regions): Likewise.
	* gnu-nat.c (gnu_find_memory_regions): Likewise.
	* linux-nat.c (linux_nat_find_memory_regions): Likewise.
	* procfs.c (iterate_over_mappings_cb_ftype): Remove.
	(iterate_over_mappings): Rename iterate_over_mappings_cb_ftype to
	find_memory_region_ftype.
	(insert_dbx_link_bpt_in_region): Likewise.
	(iterate_over_mappings): Likewise.  Drop the comment part about the
	function prototype.
	(find_memory_regions_callback): Use find_memory_region_ftype.
	(proc_find_memory_regions): Likewise.
	(info_mappings_callback): Rename iterate_over_mappings_cb_ftype to
	find_memory_region_ftype.
	* target.c (dummy_find_memory_regions): Use find_memory_region_ftype.
	* target.h (struct target_ops) <to_find_memory_regions>: Likewise.

--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -634,12 +634,13 @@ extern void init_source_path (void);
 
 /* From exec.c */
 
+typedef int (*find_memory_region_ftype) (CORE_ADDR addr, unsigned long size,
+					 int read, int write, int exec,
+					 void *data);
+
 /* Take over the 'find_mapped_memory' vector from exec.c. */
-extern void exec_set_find_memory_regions (int (*) (int (*) (CORE_ADDR, 
-							    unsigned long, 
-							    int, int, int, 
-							    void *),
-						   void *));
+extern void exec_set_find_memory_regions
+  (int (*func) (find_memory_region_ftype func, void *data));
 
 /* Possible lvalue types.  Like enum language, this should be in
    value.h, but needs to be here for the same reason. */
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -835,11 +835,7 @@ exec_has_memory (struct target_ops *ops)
 /* Find mapped memory. */
 
 extern void
-exec_set_find_memory_regions (int (*func) (int (*) (CORE_ADDR, 
-						    unsigned long, 
-						    int, int, int, 
-						    void *),
-					   void *))
+exec_set_find_memory_regions (int (*func) (find_memory_region_ftype, void *))
 {
   exec_ops.to_find_memory_regions = func;
 }
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -92,9 +92,7 @@ fbsd_read_mapping (FILE *mapfile, unsigned long *start, unsigned long *end,
    argument to FUNC.  */
 
 int
-fbsd_find_memory_regions (int (*func) (CORE_ADDR, unsigned long,
-				       int, int, int, void *),
-			  void *obfd)
+fbsd_find_memory_regions (find_memory_region_ftype func, void *obfd)
 {
   pid_t pid = ptid_get_pid (inferior_ptid);
   char *mapfilename;
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -462,9 +462,7 @@ gcore_create_callback (CORE_ADDR vaddr, unsigned long size,
 }
 
 static int
-objfile_find_memory_regions (int (*func) (CORE_ADDR, unsigned long,
-					  int, int, int, void *),
-			     void *obfd)
+objfile_find_memory_regions (find_memory_region_ftype func, void *obfd)
 {
   /* Use objfile data to create memory sections.  */
   struct objfile *objfile;
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -2487,11 +2487,7 @@ gnu_xfer_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len, int write,
 
 /* Call FUNC on each memory region in the task.  */
 static int
-gnu_find_memory_regions (int (*func) (CORE_ADDR,
-				      unsigned long,
-				      int, int, int,
-				      void *),
-			 void *data)
+gnu_find_memory_regions (find_memory_region_ftype func, void *data)
 {
   error_t err;
   task_t task;
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4104,9 +4104,7 @@ read_mapping (FILE *mapfile,
    regions in the inferior for a corefile.  */
 
 static int
-linux_nat_find_memory_regions (int (*func) (CORE_ADDR,
-					    unsigned long,
-					    int, int, int, void *), void *obfd)
+linux_nat_find_memory_regions (find_memory_region_ftype func, void *obfd)
 {
   int pid = PIDGET (inferior_ptid);
   char mapsfilename[MAXPATHLEN];
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -438,17 +438,9 @@ static void free_syscalls (procinfo *pi);
 static int find_syscall (procinfo *pi, char *name);
 #endif /* DYNAMIC_SYSCALLS */
 
-/* A function type used as a callback back iterate_over_mappings.  */
-typedef int (iterate_over_mappings_cb_ftype)
-  (CORE_ADDR vaddr, unsigned long size, int read, int write, int execute,
-   void *data);
-
 static int iterate_over_mappings
-  (procinfo *pi,
-   iterate_over_mappings_cb_ftype *child_func,
-   void *data,
-   int (*func) (struct prmap *map,
-		iterate_over_mappings_cb_ftype *child_func,
+  (procinfo *pi, find_memory_region_ftype child_func, void *data,
+   int (*func) (struct prmap *map, find_memory_region_ftype child_func,
 		void *data));
 
 /* The head of the procinfo list: */
@@ -3784,7 +3776,7 @@ solib_mappings_callback (struct prmap *map, int (*func) (int, CORE_ADDR),
 
 static int
 insert_dbx_link_bpt_in_region (struct prmap *map,
-			       iterate_over_mappings_cb_ftype *child_func,
+			       find_memory_region_ftype child_func,
 			       void *data)
 {
   procinfo *pi = (procinfo *) data;
@@ -5226,11 +5218,10 @@ procfs_use_watchpoints (struct target_ops *t)
    from the callback function, or zero.  */
 
 static int
-iterate_over_mappings (procinfo *pi,
-		       iterate_over_mappings_cb_ftype *child_func,
+iterate_over_mappings (procinfo *pi, find_memory_region_ftype child_func,
 		       void *data,
 		       int (*func) (struct prmap *map,
-				    iterate_over_mappings_cb_ftype *child_func,
+				    find_memory_region_ftype child_func,
 				    void *data))
 {
   char pathname[MAX_PROC_NAME_SIZE];
@@ -5282,23 +5273,12 @@ iterate_over_mappings (procinfo *pi,
 }
 
 /* Implements the to_find_memory_regions method.  Calls an external
-   function for each memory region.  The external function will have
-   the signature:
-
-     int callback (CORE_ADDR vaddr,
-		   unsigned long size,
-		   int read, int write, int execute,
-		   void *data);
-
+   function for each memory region.
    Returns the integer value returned by the callback.  */
 
 static int
 find_memory_regions_callback (struct prmap *map,
-			      int (*func) (CORE_ADDR,
-					   unsigned long,
-					   int, int, int,
-					   void *),
-			      void *data)
+			      find_memory_region_ftype func, void *data)
 {
   return (*func) ((CORE_ADDR) map->pr_vaddr,
 		  map->pr_size,
@@ -5321,11 +5301,7 @@ find_memory_regions_callback (struct prmap *map,
    the callback.  */
 
 static int
-proc_find_memory_regions (int (*func) (CORE_ADDR,
-				       unsigned long,
-				       int, int, int,
-				       void *),
-			  void *data)
+proc_find_memory_regions (find_memory_region_ftype func, void *data)
 {
   procinfo *pi = find_procinfo_or_die (PIDGET (inferior_ptid), 0);
 
@@ -5364,8 +5340,7 @@ mappingflags (long flags)
    mappings'.  */
 
 static int
-info_mappings_callback (struct prmap *map,
-			iterate_over_mappings_cb_ftype *ignore,
+info_mappings_callback (struct prmap *map, find_memory_region_ftype ignore,
 			void *unused)
 {
   unsigned int pr_off;
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3005,7 +3005,7 @@ dummy_pid_to_str (struct target_ops *ops, ptid_t ptid)
 
 /* Error-catcher for target_find_memory_regions.  */
 static int
-dummy_find_memory_regions (int (*ignore1) (), void *ignore2)
+dummy_find_memory_regions (find_memory_region_ftype ignore1, void *ignore2)
 {
   error (_("Command not implemented for this target."));
   return 0;
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -504,11 +504,7 @@ struct target_ops
     int (*to_async_mask) (int);
     int (*to_supports_non_stop) (void);
     /* find_memory_regions support method for gcore */
-    int (*to_find_memory_regions) (int (*) (CORE_ADDR,
-					    unsigned long,
-					    int, int, int,
-					    void *),
-				   void *);
+    int (*to_find_memory_regions) (find_memory_region_ftype func, void *data);
     /* make_corefile_notes support method for gcore */
     char * (*to_make_corefile_notes) (bfd *, int *);
     /* get_bookmark support method for bookmarks */

Re: [patch] Code cleanup: Make function typedef for find memory region

[Joel Brobecker]

> gdb/
> 2010-08-30  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Code cleanup.
> 	* defs.h (find_memory_region_t): New typedef.
> 	(exec_set_find_memory_regions): Use it.
> 	* exec.c (exec_set_find_memory_regions): Use find_memory_region_t.
> 	* fbsd-nat.c (fbsd_find_memory_regions): Likewise.
> 	* gcore.c (objfile_find_memory_regions): Likewise.
> 	* gnu-nat.c (gnu_find_memory_regions): Likewise.
> 	* linux-nat.c (linux_nat_find_memory_regions): Likewise.
> 	* procfs.c (iterate_over_mappings_cb_ftype): Remove.
> 	(iterate_over_mappings): Rename iterate_over_mappings_cb_ftype to
> 	find_memory_region_t.
> 	(insert_dbx_link_bpt_in_region): Likewise.
> 	(iterate_over_mappings): Likewise.  Drop the comment part about the
> 	function prototype.
> 	(find_memory_regions_callback): Use find_memory_region_t.
> 	(proc_find_memory_regions): Likewise.
> 	(info_mappings_callback): Rename iterate_over_mappings_cb_ftype to
> 	find_memory_region_t.
> 	* target.c (dummy_find_memory_regions): Use find_memory_region_t.
> 	* target.h (struct target_ops) <to_find_memory_regions>: Likewise.

Given the various comments so far, this patch is OK with the type
renamed to find_memory_region_ftype.


-- 
Joel

Re: [patch] Code cleanup: Make function typedef for find memory region

[Jan Kratochvil]

On Tue, 31 Aug 2010 19:33:28 +0200, Joel Brobecker wrote:
> Given the various comments so far, this patch is OK with the type
> renamed to find_memory_region_ftype.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-08/msg00198.html


Thanks,
Jan

Re: [patch] Code cleanup: Make function typedef for find memory region

[Pedro Alves]

On Tuesday 31 August 2010 18:27:21, Jan Kratochvil wrote:
> On Mon, 30 Aug 2010 12:43:55 +0200, Pedro Alves wrote:
> > I suggest you don't use _t for this.  Most of our function typedefs
> > use the *_ftype prefix, a few _func, and only a couple _fn or _t.
> [...]
> >  find_memory_region_ftype
> 
> Chose this one.

Thanks.  This looks fine to me.

-- 
Pedro Alves