Re: [PATCH 1/2] gdb.threads/attach-into-signal.exp: cleanup

[Pedro Alves <palves at redhat dot com>]

On 02/17/2012 08:24 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> - build different executable files for the non-threaded and threaded
> Pedro>   cases.  This was my motivation.  I wanted to test the non-threaded
> Pedro>   case manually, but the threaded variant always clobbered the
> Pedro>   non-threaded executable.
> 
> I think this ought to be a general rule.  We need exceptions to it for
> some executable-changed cases, but I think in general different tests
> should build different executables, because this makes it easier to do
> additional checking by hand.
> 
> Pedro> +    set save_pf_prefix $pf_prefix
> Pedro> +    lappend pf_prefix "$threadtype:"
> 
> I think this should be append rather than lappend, as pf_prefix is just
> a string, not a list.

Hmm, the only thing dejagnu does with it is 'concat'.

 /usr/share/dejagnu/framework.exp:681:    global pf_prefix
 /usr/share/dejagnu/framework.exp:688:    if {[info exists pf_prefix]} {
 /usr/share/dejagnu/framework.exp:689:   set message [concat $pf_prefix " " $message]

Interesting data-point:

$ grep -rn pf_prefix *| grep append | grep lappend | wc -l
40

$ grep -rn pf_prefix *| grep append | grep append | grep -v lappend | wc -l
2

'concat' actually takes lists as arguments, so it could be said that lappend
is the correct thing to do.  'concat' also accepts strings all the same too,
and everything is a string in tcl anyway, so it could go both ways.

Hmm, having said that, with lists we can pop back one level without
saving the previous state of pf_prefix.  That is, all the
"set save_pf_prefix $pf_prefix" are really unnecessary.  So we could instead
write:

 lappend pf_prefix "$threadtype:"
 ...
 set pf_prefix [lreplace $pf_prefix end end]

or even better, hide that a bit in a couple of methods in lib/gdb.exp:

 proc push_prefix { $new_prefix} {
   lappend pf_prefix "$threadtype:"
 }

 proc pop_prefix {} {
   set pf_prefix [lreplace $pf_prefix end end]
 }

All without any extra variables.

> 
> Pedro>      if [get_compiler_info ${binfile}] {
> Pedro> +	set pf_prefix $save_pf_prefix
> Pedro>  	return -1
> Pedro>      }
> 
> I've occasionally wanted a wrapper like 'with_pf_prefix $whatever { body }'.
> But not enough to write it :)

Oh, that's a good idea!

-- 
Pedro Alves