Re: [PATCH 1/2] Minor reorganization of fetch_registers/store_registers in windows-nat.c

[Pedro Alves <palves at redhat dot com>]

On 06/25/2018 07:55 PM, Joel Brobecker wrote:
> This patch is a small reorganizational patch that splits
> do_windows_fetch_inferior_registers into two parts:
> 
>   (a) One part that first reloads the thread's context when needed,
>       and then decides based on the given register number whether
>       one register needs to be fetched or all of them.
> 
>       This part is moved to windows_nat_target::fetch_registers.
> 
>   (b) The rest of the code, which actually fetches the register value
>       and supplies it to the regcache.
> 
> A similar treatment is applied to do_windows_store_inferior_registers.
> 
> This change is preparation work for changing the way we calculate
> the location of a given register in the thread context structure,
> and should be a no op.
> 

I agree this looks clearer without the recursion.

LGTM.  A couple nits on comments below.

> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index 620e25c..c51ef8f 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -528,14 +528,59 @@ windows_delete_thread (const ptid_t &ptid, DWORD exit_code)
>      }
>  }
>  
> +/* Fetches register number R from the given windows_thread_info,
> +   and supplies its value to the given regcache.
> +
> +   This function assumes that R is either null or positive.  A failed
> +   assertion is raised if that is not true.

R is an integer, talking about "null" gave me pause.  I'd suggest
saying either "zero" instead of null, or say "assumes non-negative".

> +
> +   This function assumes that TH->RELOAD_CONTEXT is not set, meaning
> +   that the windows_thread_info has an up-to-date context.  A failed
> +   assertion is raised if that assumption is violated.  */
> +
>  static void
> -do_windows_fetch_inferior_registers (struct regcache *regcache,
> -				     windows_thread_info *th, int r)
> +windows_fetch_one_register (struct regcache *regcache,
> +			    windows_thread_info *th, int r)
>  {

>  
>  static void
> -do_windows_store_inferior_registers (const struct regcache *regcache,
> -				     windows_thread_info *th, int r)
> +windows_store_one_register (const struct regcache *regcache,
> +			    windows_thread_info *th, int r)

Add similar leading comment?

Thanks,
Pedro Alves