Re: [PATCH] fix PR-15501

[Pedro Alves <palves at redhat dot com>]

On 08/22/2013 10:36 AM, Waqas, Muhammad wrote:

> +# This proc will be able to perform test for disable/enable
> +# commads on multiple locations and breakpoints,
> +# if args will be according to below explained values.
> +# Here arg are
> +# "what" is command (disable/enable),
> +# "what_res" is for breakpoints should be enabled or not,
> +# "p1/p2" are proc(pass/fail) but must be opposite.
> +#
> +# Here arg's values
> +# If "what" = "disable" then
> +# "what_res" = "n"
> +# "p1" = "pass"
> +# "p2" = "fail".
> +#
> +# If "what" = "enable" then
> +# "what_res" = "y"
> +# "p1" = "fail"
> +# "p2" = "pass".

Thanks.  I suggest this edit then:

# Perform tests for disable/enable commands on multiple
# locations and breakpoints.
#
# WHAT     - the command to test (disable/enable),
# WHAT_RES - whether breakpoints are expected to end
#            up enabled or disabled.
# P1/P2    - proc to call (pass/fail).  Must be
#            opposites.
#
# Furthermore, arguments must follow these rules:
#
# If "what" = "disable" then
# "what_res" = "n"
# "p1" = "pass"
# "p2" = "fail".
#
# If "what" = "enable" then
# "what_res" = "y"
# "p1" = "fail"
# "p2" = "pass".
#
> +
> +proc test_ena_dis_br { what what_res p1 p2 } {
> +    global b1
> +    global b2
> +    global b3
> +    global b4
> +    global gdb_prompt
> +
> +    # Now enable/disable $b.1 $b2.1.
> +    gdb_test_no_output "$what $b1.1 $b2.1" "$what \$b1.1 \$b2.1"
> +    set test1 "${what}d \$b1.1 and \$b2.1"
> +
> +    # Now $b1.1 and $b2.1 should be enabled/disabled
> +    gdb_test_multiple "info break" "$test1" {
> +       -re "(${b1}.1)(\[^\n\r\]*)( n.*)(${b2}.1)(\[^\n\r\]*)( n.*)$gdb_prompt $" {
> +           $p1 "$test1"
> +       }
> +       -re ".*$gdb_prompt $" {
> +           $p2 "$test1"
> +       }
> +    }
> +
> +    # Now enable/disbale $b1 fooo.1, it should give error on fooo.

Typo: disbale.

> +    # Now enable/disbale $b1 fooo.1, it should give error on fooo.
> +    gdb_test "$what $b1 fooo.1" \
> +       "Bad breakpoint number 'fooo'" \
> +       "$what \$b1 fooo.1"
> +
> +    # $b1 should be enabled/disabled.
> +    gdb_test "info break" \
> +       "(${b1})(\[^\n\r]*)( $what_res.*)" \
> +       "${what}d \$b1"
> +
> +    # Here "oppos" is commmand that should be opposite of "what".
> +    set oppos "enable"
> +    set oppos_res "y"
> +
> +    if { $what == "enable" } {
> +        set oppos "disable"
> +	set oppos_res "n"

Indentation is wrong here.  If we're doing this, then
one has to wonder why does the routine expect all
these arguments:

# If "what" = "disable" then
# "what_res" = "n"
# "p1" = "pass"
# "p2" = "fail".
#
# If "what" = "enable" then
# "what_res" = "y"
# "p1" = "fail"
# "p2" = "pass".

... instead of accepting only "what", and computing
the rest from that itself.

> +

Spurious newline.

> +    }
> +
> +    gdb_test_no_output "$oppos $b3" "$oppos \$b3"
> +    gdb_test_no_output "$what $b4 $b3.1" "$what \$b4 \$b3.1"
> +    set test1 "${what}d \$b4 and \$b3.1,remain ${oppos}d \$b3"
> +
> +    # Now $b4 $b3.1 should be enabled/disabled and
> +    # $b3 should remain enabled/disabled

enabled/disabled ... enabled/disabled

That's confusing, and says practically nothing...  They're
all either enabled or disabled.  I suggest adjusting all
these comments to be written in terms of "enabled",
with the disabled state written on the right, in parens.
So that'd be:

    # Now $b4 $b3.1 should be enabled(disabled) and
    # $b3 should remain disabled(enabled).

So, when reading this, one now clearly understands
that $b3 should end up in the opposite state of$b4 $b3.1
(it that's what should happen).

> +    gdb_test_multiple "info break" "$test1" {
> +       -re "(${b3})(\[^\n\r]*)( $oppos_res.*)(${b3}.1)(\[^\n\r\]*)( n.*)(${b4})(\[^\n\r\]*)( $what_res.*)$gdb_prompt $" {
> +           $p1 "$test1"
> +       }
> +       -re "(${b3})(\[^\n\r]*)( $oppos_res.*)(${b4})(\[^\n\r\]*)( $what_res.*)$gdb_prompt $" {
> +           $p2 "$test1"
> +       }
> +    }
> +
> +    # Now enable/disable $b4.1 fooobaar and
> +    # it should give warning on fooobaar.
> +    gdb_test "$what $b4.1 fooobaar" \
> +       "warning: bad breakpoint number at or near 'fooobaar'" \
> +       "$what \$b4.1 fooobar"
> +    set test1 "${what}d \$b4.1"
> +
> +    # $b4.1 should be enabled/disbaled

Typo.  Please remember to always self review for typos.  I
think I'm noticing them at all iterations.

> +    gdb_test_multiple "info break" "$test1" {
> +        -re "(${b4}.1)(\[^\n\r\]*)( n.*)$gdb_prompt $" {
> +           $p1 "$test1"
> +       }
> +       -re ".*$gdb_prompt $" {
> +           $p2 "$test1"
> +       }
> +    }
> +}
> +
> +test_ena_dis_br "disable" "n" "pass" "fail"
> +test_ena_dis_br "enable" "y" "fail" "pass"
> +
>  gdb_exit
>  return 0

Thanks,
-- 
Pedro Alves