| Message ID | 265aef58-69d6-bfb9-c6b0-61c9d89b45b6@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [committed] Fix non-unique testnames | expand |
On Nov 30, 2020, at 8:00 AM, Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > This patch fixes a handful of tests with non-unique names which confuse > the living hell out of compare_tests, particularly if one of two tests > [x]fail while the other is [x]pass which compare_tests will flag as a > regression each and every run. Thanks. The other way to fix the issue is to fix the tools so that they never fail. :-)
On 12/4/20 2:55 PM, Mike Stump wrote: > On Nov 30, 2020, at 8:00 AM, Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >> This patch fixes a handful of tests with non-unique names which confuse >> the living hell out of compare_tests, particularly if one of two tests >> [x]fail while the other is [x]pass which compare_tests will flag as a >> regression each and every run. > Thanks. The other way to fix the issue is to fix the tools so that they never fail. :-) Yes, but either way tests should be unique. jeff
On 12/4/20 3:17 PM, Jeff Law via Gcc-patches wrote: > > > On 12/4/20 2:55 PM, Mike Stump wrote: >> On Nov 30, 2020, at 8:00 AM, Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >>> This patch fixes a handful of tests with non-unique names which confuse >>> the living hell out of compare_tests, particularly if one of two tests >>> [x]fail while the other is [x]pass which compare_tests will flag as a >>> regression each and every run. >> Thanks. The other way to fix the issue is to fix the tools so that they never fail. :-) > Yes, but either way tests should be unique. I know I introduced some (or even most) of these and I still have to change the strings and make them unique. What bothers me is that it's yet another unnecessary hoop for us to jump through. It's not documented anywhere that these things need to be unique, there's no tooling to detect when they're not, and it gets missed in code reviews. Why not instead change the test harness to do this for us? Martin
On 12/7/20 9:19 AM, Martin Sebor wrote: > On 12/4/20 3:17 PM, Jeff Law via Gcc-patches wrote: >> >> >> On 12/4/20 2:55 PM, Mike Stump wrote: >>> On Nov 30, 2020, at 8:00 AM, Jeff Law via Gcc-patches >>> <gcc-patches@gcc.gnu.org> wrote: >>>> This patch fixes a handful of tests with non-unique names which >>>> confuse >>>> the living hell out of compare_tests, particularly if one of two tests >>>> [x]fail while the other is [x]pass which compare_tests will flag as a >>>> regression each and every run. >>> Thanks. The other way to fix the issue is to fix the tools so that >>> they never fail. :-) >> Yes, but either way tests should be unique. > > I know I introduced some (or even most) of these and I still have > to change the strings and make them unique. What bothers me is > that it's yet another unnecessary hoop for us to jump through. > It's not documented anywhere that these things need to be unique, > there's no tooling to detect when they're not, and it gets missed > in code reviews. Why not instead change the test harness to do > this for us? A change to fix the test harness would certainly be appreciated. I'm a much bigger fan of having tools catch this kind of thing than hoping a reviewer remembers to look for it. jeff
diff --git a/gcc/testsuite/g++.dg/warn/Wnonnull5.C b/gcc/testsuite/g++.dg/warn/Wnonnull5.C index 8b25d2d9f86..78862d48993 100644 --- a/gcc/testsuite/g++.dg/warn/Wnonnull5.C +++ b/gcc/testsuite/g++.dg/warn/Wnonnull5.C @@ -36,7 +36,7 @@ struct S void warn_nullptr_this () { ((S*)nullptr)->f0 (""); // { dg-warning "3:'this' pointer null" "pr86568" { xfail *-*-* } } - // { dg-warning "this' pointer null" "pr86568" { target *-*-* } .-1 } + // { dg-warning "this' pointer null" "pr86568 second variant" { target *-*-* } .-1 } } void warn_null_this_cst () @@ -49,15 +49,15 @@ void warn_null_this_var () { S* null = 0; null->f2 (&null); // { dg-warning "3:'this' pointer null" "pr86568" { xfail *-*-* } } - // { dg-warning "'this' pointer null" "pr86568" { target *-*-* } .-1 } + // { dg-warning "'this' pointer null" "pr86568 second variant" { target *-*-* } .-1 } } void warn_nullptr (S s) { s.f3 (nullptr, &s); // { dg-warning "9:argument 1 null where non-null expected" "pr86568" { xfail *-*-* } } - // { dg-warning "argument 1 null where non-null expected" "pr86568" { target *-*-* } .-1 } + // { dg-warning "argument 1 null where non-null expected" "pr86568 second variant" { target *-*-* } .-1 } s.f3 (&s, nullptr); // { dg-warning "13:argument 2 null where non-null expected" "pr86568" { xfail *-*-* } } - // { dg-warning "argument 2 null where non-null expected" "pr86568" { target *-*-* } .-1 } + // { dg-warning "argument 2 null where non-null expected" "pr86568 second variant" { target *-*-* } .-1 } } @@ -72,9 +72,9 @@ void warn_null_var (S s) { void* null = 0; s.f5 (null, &s); // { dg-warning "9:argument 1 null where non-null expected" "pr86568" { xfail *-*-* } } - // { dg-warning "argument 1 null where non-null expected" "pr86568" { target *-*-* } .-1 } + // { dg-warning "argument 1 null where non-null expected" "pr86568 second variant" { target *-*-* } .-1 } s.f5 (&s, null); // { dg-warning "16:argument 2 null where non-null expected" "pr86568" { xfail *-*-* } } - // { dg-warning "argument 2 null where non-null expected" "pr86568" { target *-*-* } .-1 } + // { dg-warning "argument 2 null where non-null expected" "pr86568 second variant" { target *-*-* } .-1 } } void warn_null_cond (S s, void *null) @@ -83,9 +83,9 @@ void warn_null_cond (S s, void *null) return; s.f6 (null, &s); // { dg-warning "9:argument 1 null where non-null expected" "pr86568" { xfail *-*-* } } - // { dg-warning "argument 1 null where non-null expected" "pr86568" { target *-*-* } .-1 } + // { dg-warning "argument 1 null where non-null expected" "pr86568 second variant" { target *-*-* } .-1 } s.f6 (&s, null); // { dg-warning "13:argument 2 null where non-null expected" "pr86568" { xfail *-*-* } } - // { dg-warning "argument 2 null where non-null expected" "pr86568" { target *-*-* } .-1 } + // { dg-warning "argument 2 null where non-null expected" "pr86568 second variant" { target *-*-* } .-1 } } diff --git a/gcc/testsuite/g++.dg/warn/Wplacement-new-size-8.C b/gcc/testsuite/g++.dg/warn/Wplacement-new-size-8.C index 77bd3314a19..12cd4cda89d 100644 --- a/gcc/testsuite/g++.dg/warn/Wplacement-new-size-8.C +++ b/gcc/testsuite/g++.dg/warn/Wplacement-new-size-8.C @@ -43,7 +43,7 @@ void test_cst_off () /* Offsets are treated as signed so SIZE_MAX is indistinguishable from -1. */ char ca1[1]; // { dg-message "at offset \\d+ from 'ca1' declared here" "note" { xfail *-*-* } } - // { dg-message "at offset -1 from 'ca1' declared here" "note" { target *-*-* } .-1 } + // { dg-message "at offset -1 from 'ca1' declared here" "note second variant" { target *-*-* } .-1 } new (ca1 + SIZE_MAX) S<1>; // { dg-warning "constructing an object of type 'S<1>' and size '1' in a region of type 'char \\\[1]' and size '0'" } } }
This patch fixes a handful of tests with non-unique names which confuse the living hell out of compare_tests, particularly if one of two tests [x]fail while the other is [x]pass which compare_tests will flag as a regression each and every run. No doubt there's a lot more of these lying around and I'm going to be watching for this much more closely going forward. Committing to the trunk, Jeff commit e40fece7c9b3731f4cff060f712c132ff100cab4 Author: Jeff Law <law@redhat.com> Date: Mon Nov 30 08:59:23 2020 -0700 Fix non-unique testnames gcc/testsuite * g++.dg/warn/Wnonnull5.C: Fix non-unique testnames. * g++.dg/warn/Wplacement-new-size-8.C: Likewise.