Re: [PATCH v2 2/2] [gdb/testsuite] Use try instead of catch

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries <tdevries@suse.de> writes:

> Use try instead of catch in a few places in lib/gdb.exp.

That commit message puts a lot of effort onto the reviewers.  You really
should be explaining the motivation for this patch a little more.  Why
is this a change worth making?

> ---
>  gdb/testsuite/lib/gdb.exp | 243 +++++++++++++++++++++++---------------
>  1 file changed, 147 insertions(+), 96 deletions(-)
>
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 2cfdbdda09d..e1b59b7a69b 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -2719,7 +2720,10 @@ proc spawn_capture_tty_name { args } {
>      # if it doesn't work, we want to be notified of that fact via the
>      # normal Tcl error reporting mechanisms.)

The comment above here needs updating.  It specifically says: "catch" is
used here because ....  And is now out of date.


>      if {[tcl_version_at_least 9 0 0]} {
> -	catch {fconfigure $spawn_id -encoding utf-8 -profile replace}
> +	try {
> +	    fconfigure $spawn_id -encoding utf-8 -profile replace
> +	} on error {} {
> +	}

This doesn't seem like an improvement.  Is there some reason why the one
line catch is not as good as the try with an empty on error block?

>      }
>      return $result
>  }

> @@ -9538,22 +9550,26 @@ proc get_build_id { filename } {
>      if { ([istarget "*-*-mingw*"]
>  	  || [istarget *-*-cygwin*]) } {
>  	set objdump_program [gdb_find_objdump]
> -	set result [catch {set data [exec $objdump_program -p $filename | grep signature | cut "-d " -f4]} output]
> -	verbose "result is $result"
> -	verbose "output is $output"
> -	if {$result == 1} {
> +	try {
> +	    set data [exec $objdump_program -p $filename | grep signature | cut "-d " -f4]
> +	} on error {msg} {
> +	    verbose "result is $msg"

It's not really a "result" now, better might be:

  verbose -log "error is: $msg"

this clearly marks it as an error, and also ensures it's always written
to the log, not just when running in verbose mode.  Though this does
make the assumption that this error path is unlikely to occur.  If you
think that some of these paths will be hit regularly then I would agree
with leaving it as just 'verbose "error is: $msg"', no point filling the
logs unnecessarily.

I think this pattern, calling an 'error' a 'result' occurs a few times.

>  	    return ""
>  	}
> +	verbose "output is $data"
>  	return $data
>      } else {
>  	set tmp [standard_output_file "${filename}-tmp"]
>  	set objcopy_program [gdb_find_objcopy]
> -	set result [catch {exec $objcopy_program -j .note.gnu.build-id -O binary $filename $tmp} output]
> -	verbose "result is $result"
> -	verbose "output is $output"
> -	if {$result == 1} {
> +	try {
> +	    set output \
> +		[exec $objcopy_program -j .note.gnu.build-id -O binary $filename $tmp]
> +	} on error {msg} {
> +	    verbose "result is $msg"
>  	    return ""
>  	}
> +	verbose "output is $output"
> +
>  	set fi [open $tmp]
>  	fconfigure $fi -translation binary
>  	# Skip the NOTE header.


> @@ -11004,12 +11041,11 @@ proc cmp_file_string { file str msg } {
>  	return
>      }
>  
> -    set caught_error [catch {
> +    try {
>  	set fp [open "$file" r]
>  	set file_contents [read $fp]
>  	close $fp
> -    } error_message]
> -    if {$caught_error} {
> +    } on error {error_message} {
>  	error "$error_message"
>  	fail "$msg"
>  	return

This is a pre-existing bug, but given you're touching this area, could
you remove the fail and return please, these are dead code.

In fact, isn't this the same as the cleanup in gdb_get_line_number,
where we catch an error only to immediately rethrow it?  Couldn't we
just drop the both catch and not add the try here?


Thanks,
Andrew