This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Introduce utility function find_inferior_ptid


> This patch introduces find_inferior_ptid to replace the common idiom
> 
>   find_inferior_pid (ptid_get_pid (...));
> 
> It replaces all the instances of that idiom that I found with the new
> function.
> 
> The change was mostly mechanical and built-tested.
> 
> gdb/ChangeLog:
> 
> 	* inferior.c (find_inferior_ptid): New function.
> 	* inferior.h (find_inferior_ptid): New declaration.
> 	* ada-tasks.c (ada_get_task_number): Use find_inferior_ptid.
> 	* corelow.c (core_pid_to_str): Same.
> 	* darwin-nat.c (darwin_resume): Same.
> 	* infrun.c (fetch_inferior_event): Same.
> 	(get_inferior_stop_soon): Same.
> 	(handle_inferior_event): Same.
> 	(handle_signal_stop): Same.
> 	* linux-nat.c (resume_lwp): Same.
> 	(stop_wait_callback): Same.
> 	* mi/mi-interp.c (mi_new_thread): Same.
> 	(mi_thread_exit): Same.
> 	* proc-service.c (ps_pglobal_lookup): Same.
> 	* record-btrace.c (record_btrace_step_thread): Same.
> 	* remote-sim.c (gdbsim_close_inferior): Same.
> 	(gdbsim_resume): Same.
> 	(gdbsim_stop): Same.
> 	* sol2-tdep.c (sol2_core_pid_to_str): Same.
> 	* target.c (memory_xfer_partial_1): Same.
> 	(default_thread_address_space): Same.
> 	* thread.c (thread_change_ptid): Same.
> 	(switch_to_thread): Same.
> 	(do_restore_current_thread_cleanup): Same.

Overall, this looks reasonable. A couple of comments below.
Also, while at it, I would prefer if you tested the change.
With the number of mechanical changes, it's really easy to
let one bad change slip by, no matter how careful we try
to be; testing does not cost much so let's add that as well.

>  
> +struct inferior *
> +find_inferior_ptid (ptid_t ptid) {
> +  return find_inferior_ptid (ptid);
> +}

This function needs an introductory commend ("/* See inferior.h */").
This is something we do systematically now, even if the function's
documentation is present elsewhere. The one-line comment allows us
to know that there is a command, in incidentally where it is.

Also, the opening curly brace needs to be on the next line. Therefore:

        /* See inferior.h.  */

        struct inferior *
        find_inferior_ptid (ptid_t ptid)
        {
          return find_inferior_ptid (ptid);
        }

> +/* Search function to lookup an inferior whose pid is equal to 'ptid.pid'. */
> +extern struct inferior * find_inferior_ptid (ptid_t ptid);

No space after the '*'.

Pre-approved with those changes and after testing confirmed.

Thank you,
-- 
Joel


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]