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

[Guinevere Larsen <guinevere@redhat.com>]

On 5/26/26 6:04 AM, Schimpe, Christina wrote:
>> -----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>
Fixed all the nits! thanks for the review!
>
> 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
>

-- 
Cheers,
Guinevere Larsen
It/she