Re: [PATCH 1/2] Tests on displaying trace data in MI

[Pedro Alves <palves at redhat dot com>]

On 06/05/2013 06:30 AM, Yao Qi wrote:
> On 06/04/2013 08:11 PM, Pedro Alves wrote:
>>>> +	# Test MI command '-stack-list-locals'.
>>>> +	mi_gdb_test "-stack-list-locals 2" \
>> WDYT of writing '--simple-values' instead of '2' ?  I think it
>> makes the test a little bit more readable, not forcing one
>> to recall what does '2' mean (not just which print_values,
>> but realizing it's a print_values, even).
>>
> 
> I don't have a strong opinion to it, since both '2' and
> '--simple-values' are valid and documented input options.  Use
> '--simple-values' in the new patch.

Thanks.  Yeah, really no biggie.  It just took me a minute to
realize what that '2' was when reviewing the patch.  The --foo
forms at least have the potential to spare others (or a
future self) the same.

>>>> +	# to test GDB is able to clear its local state on tracing
>>>> +	# in the following steps.
>> I had a bit of trouble figuring out what this meant.  I suggest:
>>
>>    to test that GDB is able to clear its tracing-related local state
>>    in the next -target-select.
>>
> 
> I think "local state on tracing" equals to "tracing-related local
> state", so now the comment looks like:

That's part of what didn't sound clear to me.  The variant you
propose sounds ambiguous to me with saying "clear its local state
on (the next) tracing (session done) in the following steps", which
is not what you meant.

> 
> 	# Don't issue command '-trace-find none' to return from
> 	# tfind mode (examining trace frames) on purpose, in order
> 	# to test GDB is able to clear its local state on tracing
> 	# in the next -target-select.

I think the suggested "that" before "GDB" makes it sound more natural.

Anyway, as you know, I'm not a native speaker either.  :-)

> +
> +# Change target to ctf if GDB supports it.
> +send_gdb "-target-select ctf ${tracefile}.ctf\n"
> +gdb_expect {
> +    -re ".*\\^connected.*${mi_gdb_prompt}$" {
> +	# GDB supports ctf, do the test.
> +	test_trace_unavailable "ctf"
> +    }
> +    -re ".*\\^error,msg=\"Undefined target command.*${mi_gdb_prompt}$" {
> +	# GDB doesn't support ctf, skip the test.
> +	unsupported "gdb does not support ctf target"
> +    }
> +}
> +

Missing handling for:

    -re ".*$mi_gdb_prompt$" {
        fail "$msg"
    }
    timeout {
        fail "$msg (timeout)"
    }

Otherwise OK.

-- 
Pedro Alves