RE: [PATCH v3 4/7] gdb/record: make record_full_history more c++-like

["Schimpe, Christina" <christina.schimpe@intel.com>]

> -----Original Message-----
> From: Guinevere Larsen <guinevere@redhat.com>
> Sent: Freitag, 15. Mai 2026 18:37
> To: gdb-patches@sourceware.org
> Cc: Guinevere Larsen <guinevere@redhat.com>
> Subject: [PATCH v3 4/7] gdb/record: make record_full_history more c++-like
> 
> Move some functions to being a method of record_full_history, instead of
> floating functions.

Commit message:

- Shouldn't it be "a method of struct record_full_instruction" (also for the header)?
- it's only one function, so you could simply write the function name instead of "Move some functions" 

> ---
>  gdb/record-full.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/record-full.c b/gdb/record-full.c index
> 24e94eaf5da..8cabd6e9438 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -388,6 +388,10 @@ struct record_full_instruction
>    uint32_t insn_num;
>    std::optional<gdb_signal> sigval;
>    std::vector<record_full_entry> effects;
> +
> +  /* Execute the full instruction.  As a side effect, set
> +     record_full_stop_reason.  */
> +  void exec_insn (regcache *regcache);
>  };
> 
>  /* If true, query if PREC cannot record memory @@ -843,12 +847,9 @@
> record_full_gdb_operation_disable_set (void)  static enum target_stop_reason
> record_full_stop_reason
>    = TARGET_STOPPED_BY_NO_REASON;
> 
> -static inline void
> -record_full_exec_insn (regcache *regcache,
> -		       gdbarch *gdbarch,
> -		       record_full_instruction &insn)
> +void record_full_instruction::exec_insn (regcache *regcache)
>  {
> -  for (auto &entry : insn.effects)
> +  for (auto &entry : effects)
>      if (entry.execute (regcache))
>        record_full_stop_reason = TARGET_STOPPED_BY_WATCHPOINT;  } @@ -
> 1308,9 +1309,7 @@ record_full_wait_1 (struct target_ops *ops,
>  		  break;
>  		}
> 
> -	      record_full_exec_insn
> -		(regcache, gdbarch,
> -		 record_full_list[record_full_next_insn]);
> +	      record_full_list[record_full_next_insn].exec_insn (regcache);
> 
>  	      /* step */
>  	      if (record_full_resume_step)
> @@ -2503,8 +2502,7 @@ record_full_base_target::save_record (const char
> *recfilename)
> 
>    /* Reverse execute to the begin of record list.  */
>    for (int i = record_full_next_insn - 1; i >= 0; i--)
> -    record_full_exec_insn (regcache, gdbarch,
> -			   record_full_list[i]);
> +    record_full_list[i].exec_insn (regcache);
> 
>    /* Compute the size needed for the extra bfd section.  */
>    save_size = 4;	/* magic cookie */
> @@ -2576,7 +2574,7 @@ record_full_base_target::save_record (const char
> *recfilename)
>  	}
>        /* Execute entry.  */
>        if (i < record_full_next_insn)
> -	record_full_exec_insn (regcache, gdbarch, record_full_list[i]);
> +	record_full_list[i].exec_insn (regcache);
>      }
> 
>    unlink_file.keep ();
> @@ -2597,17 +2595,16 @@ record_full_goto_insn (size_t target_insn,
>    scoped_restore restore_operation_disable
>      = record_full_gdb_operation_disable_set ();
>    regcache *regcache = get_thread_regcache (inferior_thread ());
> -  struct gdbarch *gdbarch = regcache->arch ();
> 
>    /* Assume everything is valid: we will hit the entry,
>       and we will not hit the end of the recording.  */
> 
>    if (dir == EXEC_REVERSE)
>      for (int i = record_full_next_insn; i > target_insn; i--)
> -      record_full_exec_insn (regcache, gdbarch, record_full_list[i - 1]);
> +      record_full_list[i-1].exec_insn (regcache);

Nit: spaces missing in i-1 => i - i

>    else
>      for (int i = record_full_next_insn; i < target_insn; i++)
> -      record_full_exec_insn (regcache, gdbarch, record_full_list[i]);
> +      record_full_list[i].exec_insn (regcache);
>  }
> 
>  /* Alias for "target record-full".  */
> --
> 2.54.0
> 

With the nits fixed, LGTM.

Reviewed-By: Christina Schimpe <christina.schimpe@intel.com>

Christina

Intel Deutschland GmbH
Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928