Re: [PATCH 6/6] New test: gdb.trace/mi-trace-frame-collected.exp

[Pedro Alves <palves@redhat.com>]

On 06/13/2013 02:28 AM, Yao Qi wrote:

> +# Define an action.
> +set pcreg ""

Suggest giving it an empty line after the comment, as the set
is not yet defining the action.

> +if [is_amd64_regs_target] {
> +    set pcreg "rip"
> +} elseif [is_x86_like_target] {
> +    set pcreg "eip"
> +} else {
> +    # Other ports support tracepoing should set the name of pc
> +    # register.

    # Other ports that support tracepoints should set the name
    # of the pc register here.

> +    fail "set the number and the name of pc register"
> +    return -1
> +}
> +
> +mi_gdb_test "-break-commands 3 \"collect gdb_char_test\" \"collect gdb_union1_test\" \"collect gdb_struct1_test.l\" \"collect gdb_arr_test\[0\]\" \"collect $${pcreg}\" \"teval \$tsv += 1\" \"collect \$tsv\" \"end\" " \
> +    {\^done} "set action"
> +
> +mi_gdb_test "-break-insert -a gdb_c_test" \
> +    "\\^done,bkpt=\{number=\"${decimal}\",type=\"tracepoint\".*\"\}" \
> +    "insert tracepoint on gdb_c_test"
> +# Collect a global variable to make sure no registers are collected
> +# expect PC.

Typo: except.  I do that one often too.  :-)

Suggest s/make sure/be sure/.

> +# Test MI command '-trace-frame-collected' from different data source.

"from different" threw me off a second (different compared to what?).

Suggest:

# Test the MI command '-trace-frame-collected' with DATA_SOURCE as
# trace data source.

> +
> +proc test_trace_frame_collected { data_source } {
> +    global decimal hex
> +    global mi_gdb_prompt
> +

This looks OK to me with those changes.

Thanks!

-- 
Pedro Alves