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

Pedro Alves
2012-02-17T20:55:29Z

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