| Message ID | c9b6ef063fbb5e33c157b1041418cd2fb7d8cf27.1497466455.git.andrew.burgess@embecosm.com |
|---|---|
| State | New |
| Headers | show |
Andrew Burgess <andrew.burgess@embecosm.com> writes: > mi_gdb_test "540-data-evaluate-expression vla1(1)" \ > - "540\\^done,value=\"1\"" "evaluate filled vla" > + "540\\^done,value=\"1\"" "evaluate filled vla(1)" Do not use "tail parentheses" on the test message, https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages Maybe ""evaluate filled vla1 1"? > mi_gdb_test "550-data-evaluate-expression vla1(2)" \ > - "550\\^done,value=\"42\"" "evaluate filled vla" > + "550\\^done,value=\"42\"" "evaluate filled vla(2)" > mi_gdb_test "560-data-evaluate-expression vla1(4)" \ > - "560\\^done,value=\"24\"" "evaluate filled vla" > + "560\\^done,value=\"24\"" "evaluate filled vla(4)" Otherwise, patch is good to me.
On 06/22/2017 11:37 AM, Yao Qi wrote: > Andrew Burgess <andrew.burgess@embecosm.com> writes: > > >> mi_gdb_test "540-data-evaluate-expression vla1(1)" \ >> - "540\\^done,value=\"1\"" "evaluate filled vla" >> + "540\\^done,value=\"1\"" "evaluate filled vla(1)" > > Do not use "tail parentheses" on the test message, > https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages The wiki doesn't mention it, but I think the rule should only apply when there's a space before the parens. Otherwise, we have a problem with all the tests that call functions, and don't explicitly specify a test name, like: gdb_test "p function(1)" " = 1" We have many such cases, C tests, Python tests, etc: $ grep "[a-z](.*)$" testsuite/gdb.sum | wc -l 1174 I don't think it's worth it, or even a good idea to try to come up with different test names for all of these. In cases like these, I think it's generally possible to avoid the space before the parens. So IMO, we should clarify the rule instead (and the buildbot testresult diffing accordingly, if necessary). Thanks, Pedro Alves
On Thu, Jun 22, 2017 at 12:08 PM, Pedro Alves <palves@redhat.com> wrote: > On 06/22/2017 11:37 AM, Yao Qi wrote: >> Andrew Burgess <andrew.burgess@embecosm.com> writes: >> >> >>> mi_gdb_test "540-data-evaluate-expression vla1(1)" \ >>> - "540\\^done,value=\"1\"" "evaluate filled vla" >>> + "540\\^done,value=\"1\"" "evaluate filled vla(1)" >> >> Do not use "tail parentheses" on the test message, >> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages > > The wiki doesn't mention it, but I think the rule should only apply when > there's a space before the parens. I wanted to get confirm from analyze-racy-logs.py, but I was lost in it, so I decided to follow the wiki. Andrew, your patch is OK as-is. > > I don't think it's worth it, or even a good idea to try to > come up with different test names for all of these. In cases like > these, I think it's generally possible to avoid the space before > the parens. So IMO, we should clarify the rule instead > (and the buildbot testresult diffing accordingly, if necessary). > The current rule is "When you write a test, do not put text between parentheses at the end of the text message", we can change it "when you write a test, do not put text between parentheses at the end of the text message and space before parentheses". They are OK, PASS: gdb.base/foo.exp: whatever test FAIL: gdb.base/foo.exp: whatever test (timeout) PASS: gdb.base/foo.exp: whatever test(1st) PASS: gdb.base/foo.exp: whatever test(2nd) They are not OK, PASS: gdb.trace/trace-break.exp: 2 trace trace on: trace set_point (1) PASS: gdb.trace/trace-break.exp: 2 trace trace on: trace set_point (2) Is it right?
On 06/22/2017 12:59 PM, Yao Qi wrote: > On Thu, Jun 22, 2017 at 12:08 PM, Pedro Alves <palves@redhat.com> wrote: >> On 06/22/2017 11:37 AM, Yao Qi wrote: >>> Andrew Burgess <andrew.burgess@embecosm.com> writes: >>> >>> >>>> mi_gdb_test "540-data-evaluate-expression vla1(1)" \ >>>> - "540\\^done,value=\"1\"" "evaluate filled vla" >>>> + "540\\^done,value=\"1\"" "evaluate filled vla(1)" >>> >>> Do not use "tail parentheses" on the test message, >>> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages >> >> The wiki doesn't mention it, but I think the rule should only apply when >> there's a space before the parens. > > I wanted to get confirm from analyze-racy-logs.py, > but I was lost in it, so I decided to follow the wiki. I only realized the need for the tweak to the rule after the original discussion that led to the rule being added to the wiki, and this is the first time I'm suggesting it, I think. >> >> I don't think it's worth it, or even a good idea to try to >> come up with different test names for all of these. In cases like >> these, I think it's generally possible to avoid the space before >> the parens. So IMO, we should clarify the rule instead >> (and the buildbot testresult diffing accordingly, if necessary). >> > > The current rule is "When you write a test, do not > put text between parentheses at the end of the text > message", we can change it "when you write a test, > do not put text between parentheses at the end of > the text message and space before parentheses". > > They are OK, > PASS: gdb.base/foo.exp: whatever test > FAIL: gdb.base/foo.exp: whatever test (timeout) > PASS: gdb.base/foo.exp: whatever test(1st) > PASS: gdb.base/foo.exp: whatever test(2nd) > > They are not OK, > PASS: gdb.trace/trace-break.exp: 2 trace trace on: trace set_point (1) > PASS: gdb.trace/trace-break.exp: 2 trace trace on: trace set_point (2) > > Is it right? Yes. Thanks, Pedro Alves
On 06/14/2017 07:57 PM, Andrew Burgess wrote: > @@ -95,7 +95,7 @@ mi_run_cmd > mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \ > { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno" > mi_gdb_test "520-data-evaluate-expression vla1" \ > - "520\\^done,value=\"\\(1, 1, 1, 1, 1\\)\"" "evaluate filled vla" > + "520\\^done,value=\"\\(1, 1, 1, 1, 1\\)\"" "evaluate filled vla at line $bp_lineno" > As principle, please avoid putting line numbers in test names. This causes test names to spuriously change if/when the source file changes. Thanks, Pedro Alves
diff --git a/gdb/testsuite/gdb.mi/mi-vla-fortran.exp b/gdb/testsuite/gdb.mi/mi-vla-fortran.exp index 7b40aba..22e2bce 100644 --- a/gdb/testsuite/gdb.mi/mi-vla-fortran.exp +++ b/gdb/testsuite/gdb.mi/mi-vla-fortran.exp @@ -44,7 +44,7 @@ mi_run_cmd mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \ { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno" mi_gdb_test "500-data-evaluate-expression vla1" \ - "500\\^done,value=\"<not allocated>\"" "evaluate not allocated vla" + "500\\^done,value=\"<not allocated>\"" "evaluate not allocated vla at line $bp_lineno" mi_create_varobj_checked vla1_not_allocated vla1 "<not allocated>" \ "create local variable vla1_not_allocated" @@ -95,7 +95,7 @@ mi_run_cmd mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \ { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno" mi_gdb_test "520-data-evaluate-expression vla1" \ - "520\\^done,value=\"\\(1, 1, 1, 1, 1\\)\"" "evaluate filled vla" + "520\\^done,value=\"\\(1, 1, 1, 1, 1\\)\"" "evaluate filled vla at line $bp_lineno" set bp_lineno [gdb_get_line_number "vla1-modified"] @@ -106,13 +106,13 @@ mi_run_cmd mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \ { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno" mi_gdb_test "530-data-evaluate-expression vla1" \ - "530\\^done,value=\"\\(1, 42, 1, 24, 1\\)\"" "evaluate filled vla" + "530\\^done,value=\"\\(1, 42, 1, 24, 1\\)\"" "evaluate filled vla at line $bp_lineno" mi_gdb_test "540-data-evaluate-expression vla1(1)" \ - "540\\^done,value=\"1\"" "evaluate filled vla" + "540\\^done,value=\"1\"" "evaluate filled vla(1)" mi_gdb_test "550-data-evaluate-expression vla1(2)" \ - "550\\^done,value=\"42\"" "evaluate filled vla" + "550\\^done,value=\"42\"" "evaluate filled vla(2)" mi_gdb_test "560-data-evaluate-expression vla1(4)" \ - "560\\^done,value=\"24\"" "evaluate filled vla" + "560\\^done,value=\"24\"" "evaluate filled vla(4)" set bp_lineno [gdb_get_line_number "vla1-deallocated"] @@ -123,7 +123,7 @@ mi_run_cmd mi_expect_stop "breakpoint-hit" "vla" "" ".*vla.f90" "$bp_lineno" \ { "" "disp=\"del\"" } "run to breakpoint at line $bp_lineno" mi_gdb_test "570-data-evaluate-expression vla1" \ - "570\\^done,value=\"<not allocated>\"" "evaluate not allocated vla" + "570\\^done,value=\"<not allocated>\"" "evaluate not allocated vla at line $bp_lineno" set bp_lineno [gdb_get_line_number "pvla2-not-associated"]