Re: [PING**4] [PATCH] Guile: temporary breakpoints

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-06-08 10:21 p.m., Simon Marchi via Gdb-patches wrote:
> On 2021-06-08 5:00 p.m., Keith Seitz via Gdb-patches wrote:
>> Hi,
>>
>> I apologize that no one has yet reviewed your patch, which certainly seems
>> like something we would want. Feature parity between Python and Guile can
>> only be good for users.
>>
>> I've nearly no experience with Guile, but looking this over, this patch looks
>> pretty straightforward, but I do have some small suggestions/requests below.
>>
>> Otherwise, I would like to recommend that a maintainer approve this.
>> [I am not someone that can grant permission to push to the repo.]
>>
>> Thanks again for the patch *and* your patience.
>>
>> Keith
> 
> Thanks for looking at it Keith, it helps a lot.
> 
>>>>>>> +proc_with_prefix test_bkpt_temporary { } {
>>>>>>> +    global srcfile testfile hex decimal
>>>>>>> +
>>>>>>> +    with_test_prefix test_bkpt_temporary {
>>>>>>> +	# Start with a fresh gdb.
>>>>>>> +	clean_restart ${testfile}
>>>>>>> +
>>>>>>> +	if ![gdb_guile_runto_main] then {
>>>>>>> +	    fail "cannot run to main."
>>
>> For consistency, please use "if {expr} {".
>>
>> This result should probably be 'untested "cannot run to main"' instead
>> of simply a fail. [Taken straight from gdb.base/template.exp.]
> 
> Perhaps it was a mistake to use untested in gdb.base/template.exp?
> 
> There are more instances of using "fail":
> 
> 
>     $ ag -A 2 runto_main | grep untested | wc -l
>     198
>     $ ag -A 2 runto_main | grep fail | wc -l
>     555
> 
> ... and it makes more sense to me to use "fail".  If I introduce a bug
> that prevents running to main to work, untested would be easier to miss
> than fail.
> 
> Otherwise, I don't have the original context to quote but where it says:
> 
>   return scm_from_bool (bp_smob->bp->disposition == disp_del ||
> 			bp_smob->bp->disposition == disp_del_at_next_stop);
> 
> put the || operator on the next line:
> 
>   return scm_from_bool (bp_smob->bp->disposition == disp_del
> 			|| bp_smob->bp->disposition == disp_del_at_next_stop);
> 
> 
> Other than that, the patch LGTM after taking Keith's feedback in
> consideration.
> 
> Simon
> 

Oh also, we probably need a short NEWS entry for this.

Simon