| Message ID | 20240716023631.906098-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: missing -Wunused-value for !<expr> [PR114104] | expand |
On Mon, Jul 15, 2024 at 10:37 PM Patrick Palka <ppalka@redhat.com> wrote: > > Bootstrapped andrregtested on x86_64-pc-linux-gnu, does this look > OK for trunk? > > -- >8 -- > > Here we're neglecting to emit a -Wunused-value for eligible ! operator > expressions, and in turn for != operator expressions that are rewritten > as !(x == y), only because we don't call warn_if_unused_value on > TRUTH_NOT_EXPR since its class is tcc_expression. This patch makes us > consider warning for TRUTH_NOT_EXPR as well. > Eh, I think I've seen the ! operator recommended as a way to silence -Wunused-value previously in cases where an actual fix isn't practical; some people might be mad about this... > PR c++/114104 > > gcc/cp/ChangeLog: > > * cvt.cc (convert_to_void): Call warn_if_unused_value for > TRUTH_NOT_EXPR as well. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wunused-20.C: New test. > --- > gcc/cp/cvt.cc | 1 + > gcc/testsuite/g++.dg/warn/Wunused-20.C | 17 +++++++++++++++++ > 2 files changed, 18 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/warn/Wunused-20.C > > diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc > index db086c017e8..ebff6db2ea9 100644 > --- a/gcc/cp/cvt.cc > +++ b/gcc/cp/cvt.cc > @@ -1664,6 +1664,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain) > if (tclass == tcc_comparison > || tclass == tcc_unary > || tclass == tcc_binary > + || code == TRUTH_NOT_EXPR > || code == VEC_PERM_EXPR > || code == VEC_COND_EXPR) > warn_if_unused_value (e, loc); > diff --git a/gcc/testsuite/g++.dg/warn/Wunused-20.C b/gcc/testsuite/g++.dg/warn/Wunused-20.C > new file mode 100644 > index 00000000000..6d7838fab13 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wunused-20.C > @@ -0,0 +1,17 @@ > +// PR c++/114104 > +// { dg-additional-options "-Wunused-value" } > + > +bool f(); > + > +void g() { > + !f(); // { dg-warning "value computed is not used" } > +} > + > +#if __cplusplus >= 202002L > +struct A { }; > +[[nodiscard]] bool operator==(A, int); > + > +void g(A a) { > + a != 0; // { dg-warning "value computed is not used" "" { target c++20 } } > +} > +#endif > -- > 2.46.0.rc0.35.gad850ef1cf >
On Tue, 16 Jul 2024, Eric Gallager wrote: > On Mon, Jul 15, 2024 at 10:37 PM Patrick Palka <ppalka@redhat.com> wrote: > > > > Bootstrapped andrregtested on x86_64-pc-linux-gnu, does this look > > OK for trunk? > > > > -- >8 -- > > > > Here we're neglecting to emit a -Wunused-value for eligible ! operator > > expressions, and in turn for != operator expressions that are rewritten > > as !(x == y), only because we don't call warn_if_unused_value on > > TRUTH_NOT_EXPR since its class is tcc_expression. This patch makes us > > consider warning for TRUTH_NOT_EXPR as well. > > > > Eh, I think I've seen the ! operator recommended as a way to silence > -Wunused-value previously in cases where an actual fix isn't > practical; some people might be mad about this... That's surprising, because even the C FE emits a -Wunused-value warning for '!f();'. Other compilers also diagnose it. The standard way to silence the warning, which works even for non-integral expressions, is to just cast to void I think. See https://gcc.gnu.org/PR114104#c1 for a realistic bug that we would now diagnose with this patch. > > > PR c++/114104 > > > > gcc/cp/ChangeLog: > > > > * cvt.cc (convert_to_void): Call warn_if_unused_value for > > TRUTH_NOT_EXPR as well. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/warn/Wunused-20.C: New test. > > --- > > gcc/cp/cvt.cc | 1 + > > gcc/testsuite/g++.dg/warn/Wunused-20.C | 17 +++++++++++++++++ > > 2 files changed, 18 insertions(+) > > create mode 100644 gcc/testsuite/g++.dg/warn/Wunused-20.C > > > > diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc > > index db086c017e8..ebff6db2ea9 100644 > > --- a/gcc/cp/cvt.cc > > +++ b/gcc/cp/cvt.cc > > @@ -1664,6 +1664,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain) > > if (tclass == tcc_comparison > > || tclass == tcc_unary > > || tclass == tcc_binary > > + || code == TRUTH_NOT_EXPR > > || code == VEC_PERM_EXPR > > || code == VEC_COND_EXPR) > > warn_if_unused_value (e, loc); > > diff --git a/gcc/testsuite/g++.dg/warn/Wunused-20.C b/gcc/testsuite/g++.dg/warn/Wunused-20.C > > new file mode 100644 > > index 00000000000..6d7838fab13 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/warn/Wunused-20.C > > @@ -0,0 +1,17 @@ > > +// PR c++/114104 > > +// { dg-additional-options "-Wunused-value" } > > + > > +bool f(); > > + > > +void g() { > > + !f(); // { dg-warning "value computed is not used" } > > +} > > + > > +#if __cplusplus >= 202002L > > +struct A { }; > > +[[nodiscard]] bool operator==(A, int); > > + > > +void g(A a) { > > + a != 0; // { dg-warning "value computed is not used" "" { target c++20 } } > > +} > > +#endif > > -- > > 2.46.0.rc0.35.gad850ef1cf > > > >
On 7/16/24 10:31 AM, Eric Gallager wrote: > On Mon, Jul 15, 2024 at 10:37 PM Patrick Palka <ppalka@redhat.com> wrote: >> >> Bootstrapped andrregtested on x86_64-pc-linux-gnu, does this look >> OK for trunk? >> >> -- >8 -- >> >> Here we're neglecting to emit a -Wunused-value for eligible ! operator >> expressions, and in turn for != operator expressions that are rewritten >> as !(x == y), only because we don't call warn_if_unused_value on >> TRUTH_NOT_EXPR since its class is tcc_expression. This patch makes us >> consider warning for TRUTH_NOT_EXPR as well. >> > > Eh, I think I've seen the ! operator recommended as a way to silence > -Wunused-value previously in cases where an actual fix isn't > practical; some people might be mad about this... That sounds like weird advice to me; I'd expect the result of ! to be used, and we already warn if the operand is something other than bool. Clang also warns for the bool case. Seems like ADDR_EXPR is another tcc_expression that could use handling here, e.g. struct A { int i; }; A& f(); int main() { &f().i; // missed warning } Jason
On Tue, Jul 16, 2024 at 10:50 AM Jason Merrill <jason@redhat.com> wrote: > > On 7/16/24 10:31 AM, Eric Gallager wrote: > > On Mon, Jul 15, 2024 at 10:37 PM Patrick Palka <ppalka@redhat.com> wrote: > >> > >> Bootstrapped andrregtested on x86_64-pc-linux-gnu, does this look > >> OK for trunk? > >> > >> -- >8 -- > >> > >> Here we're neglecting to emit a -Wunused-value for eligible ! operator > >> expressions, and in turn for != operator expressions that are rewritten > >> as !(x == y), only because we don't call warn_if_unused_value on > >> TRUTH_NOT_EXPR since its class is tcc_expression. This patch makes us > >> consider warning for TRUTH_NOT_EXPR as well. > >> > > > > Eh, I think I've seen the ! operator recommended as a way to silence > > -Wunused-value previously in cases where an actual fix isn't > > practical; some people might be mad about this... > > That sounds like weird advice to me; I'd expect the result of ! to be > used, and we already warn if the operand is something other than bool. > Clang also warns for the bool case. > It's possible that I was getting -Wunused-value confused with -Wunused-result when I was remembering the advice I'd heard: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425 > Seems like ADDR_EXPR is another tcc_expression that could use handling > here, e.g. > > struct A { int i; }; > A& f(); > int main() { > &f().i; // missed warning > } > > Jason >
On Tue, 16 Jul 2024, Jason Merrill wrote: > On 7/16/24 10:31 AM, Eric Gallager wrote: > > On Mon, Jul 15, 2024 at 10:37 PM Patrick Palka <ppalka@redhat.com> wrote: > > > > > > Bootstrapped andrregtested on x86_64-pc-linux-gnu, does this look > > > OK for trunk? > > > > > > -- >8 -- > > > > > > Here we're neglecting to emit a -Wunused-value for eligible ! operator > > > expressions, and in turn for != operator expressions that are rewritten > > > as !(x == y), only because we don't call warn_if_unused_value on > > > TRUTH_NOT_EXPR since its class is tcc_expression. This patch makes us > > > consider warning for TRUTH_NOT_EXPR as well. > > > > > > > Eh, I think I've seen the ! operator recommended as a way to silence > > -Wunused-value previously in cases where an actual fix isn't > > practical; some people might be mad about this... > > That sounds like weird advice to me; I'd expect the result of ! to be used, > and we already warn if the operand is something other than bool. Clang also > warns for the bool case. > > Seems like ADDR_EXPR is another tcc_expression that could use handling here, > e.g. > > struct A { int i; }; > A& f(); > int main() { > &f().i; // missed warning > } Makes sense, like so? Bootstrapped and regtested on x86_64-pc-linux-gnu -- >8 -- Subject: [PATCH] c++: missing -Wunused-value for !<expr> [PR114104] Here we're neglecting to emit a -Wunused-value for eligible ! operator expressions, and in turn for != operator expressions that are rewritten as !(x == y), only because we don't call warn_if_unused_value on TRUTH_NOT_EXPR since its class is tcc_expression. This patch makes us consider warning for TRUTH_NOT_EXPR as well, along with ADDR_EXPR. PR c++/114104 gcc/cp/ChangeLog: * cvt.cc (convert_to_void): Call warn_if_unused_value for TRUTH_NOT_EXPR and ADDR_EXPR as well. gcc/testsuite/ChangeLog: * g++.dg/warn/Wunused-20.C: New test. --- gcc/cp/cvt.cc | 2 ++ gcc/testsuite/g++.dg/warn/Wunused-20.C | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 gcc/testsuite/g++.dg/warn/Wunused-20.C diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc index db086c017e8..d95e01c118c 100644 --- a/gcc/cp/cvt.cc +++ b/gcc/cp/cvt.cc @@ -1664,6 +1664,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain) if (tclass == tcc_comparison || tclass == tcc_unary || tclass == tcc_binary + || code == TRUTH_NOT_EXPR + || code == ADDR_EXPR || code == VEC_PERM_EXPR || code == VEC_COND_EXPR) warn_if_unused_value (e, loc); diff --git a/gcc/testsuite/g++.dg/warn/Wunused-20.C b/gcc/testsuite/g++.dg/warn/Wunused-20.C new file mode 100644 index 00000000000..31b1920adcd --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wunused-20.C @@ -0,0 +1,19 @@ +// PR c++/114104 +// { dg-additional-options "-Wunused-value" } + +bool f(); +struct A { int i; }; +A& g(); + +void h() { + !f(); // { dg-warning "value computed is not used" } + &g().i; // { dg-warning "value computed is not used" } +} + +#if __cplusplus >= 202002L +[[nodiscard]] bool operator==(A&, int); + +void h(A a) { + a != 0; // { dg-warning "value computed is not used" "" { target c++20 } } +} +#endif
On 7/17/24 3:20 PM, Patrick Palka wrote: > On Tue, 16 Jul 2024, Jason Merrill wrote: > >> On 7/16/24 10:31 AM, Eric Gallager wrote: >>> On Mon, Jul 15, 2024 at 10:37 PM Patrick Palka <ppalka@redhat.com> wrote: >>>> >>>> Bootstrapped andrregtested on x86_64-pc-linux-gnu, does this look >>>> OK for trunk? >>>> >>>> -- >8 -- >>>> >>>> Here we're neglecting to emit a -Wunused-value for eligible ! operator >>>> expressions, and in turn for != operator expressions that are rewritten >>>> as !(x == y), only because we don't call warn_if_unused_value on >>>> TRUTH_NOT_EXPR since its class is tcc_expression. This patch makes us >>>> consider warning for TRUTH_NOT_EXPR as well. >>>> >>> >>> Eh, I think I've seen the ! operator recommended as a way to silence >>> -Wunused-value previously in cases where an actual fix isn't >>> practical; some people might be mad about this... >> >> That sounds like weird advice to me; I'd expect the result of ! to be used, >> and we already warn if the operand is something other than bool. Clang also >> warns for the bool case. >> >> Seems like ADDR_EXPR is another tcc_expression that could use handling here, >> e.g. >> >> struct A { int i; }; >> A& f(); >> int main() { >> &f().i; // missed warning >> } > > Makes sense, like so? Bootstrapped and regtested on x86_64-pc-linux-gnu OK. > -- >8 -- > > Subject: [PATCH] c++: missing -Wunused-value for !<expr> [PR114104] > > Here we're neglecting to emit a -Wunused-value for eligible ! operator > expressions, and in turn for != operator expressions that are rewritten > as !(x == y), only because we don't call warn_if_unused_value on > TRUTH_NOT_EXPR since its class is tcc_expression. This patch makes us > consider warning for TRUTH_NOT_EXPR as well, along with ADDR_EXPR. > > PR c++/114104 > > gcc/cp/ChangeLog: > > * cvt.cc (convert_to_void): Call warn_if_unused_value for > TRUTH_NOT_EXPR and ADDR_EXPR as well. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wunused-20.C: New test. > --- > gcc/cp/cvt.cc | 2 ++ > gcc/testsuite/g++.dg/warn/Wunused-20.C | 19 +++++++++++++++++++ > 2 files changed, 21 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/warn/Wunused-20.C > > diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc > index db086c017e8..d95e01c118c 100644 > --- a/gcc/cp/cvt.cc > +++ b/gcc/cp/cvt.cc > @@ -1664,6 +1664,8 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain) > if (tclass == tcc_comparison > || tclass == tcc_unary > || tclass == tcc_binary > + || code == TRUTH_NOT_EXPR > + || code == ADDR_EXPR > || code == VEC_PERM_EXPR > || code == VEC_COND_EXPR) > warn_if_unused_value (e, loc); > diff --git a/gcc/testsuite/g++.dg/warn/Wunused-20.C b/gcc/testsuite/g++.dg/warn/Wunused-20.C > new file mode 100644 > index 00000000000..31b1920adcd > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wunused-20.C > @@ -0,0 +1,19 @@ > +// PR c++/114104 > +// { dg-additional-options "-Wunused-value" } > + > +bool f(); > +struct A { int i; }; > +A& g(); > + > +void h() { > + !f(); // { dg-warning "value computed is not used" } > + &g().i; // { dg-warning "value computed is not used" } > +} > + > +#if __cplusplus >= 202002L > +[[nodiscard]] bool operator==(A&, int); > + > +void h(A a) { > + a != 0; // { dg-warning "value computed is not used" "" { target c++20 } } > +} > +#endif
diff --git a/gcc/cp/cvt.cc b/gcc/cp/cvt.cc index db086c017e8..ebff6db2ea9 100644 --- a/gcc/cp/cvt.cc +++ b/gcc/cp/cvt.cc @@ -1664,6 +1664,7 @@ convert_to_void (tree expr, impl_conv_void implicit, tsubst_flags_t complain) if (tclass == tcc_comparison || tclass == tcc_unary || tclass == tcc_binary + || code == TRUTH_NOT_EXPR || code == VEC_PERM_EXPR || code == VEC_COND_EXPR) warn_if_unused_value (e, loc); diff --git a/gcc/testsuite/g++.dg/warn/Wunused-20.C b/gcc/testsuite/g++.dg/warn/Wunused-20.C new file mode 100644 index 00000000000..6d7838fab13 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wunused-20.C @@ -0,0 +1,17 @@ +// PR c++/114104 +// { dg-additional-options "-Wunused-value" } + +bool f(); + +void g() { + !f(); // { dg-warning "value computed is not used" } +} + +#if __cplusplus >= 202002L +struct A { }; +[[nodiscard]] bool operator==(A, int); + +void g(A a) { + a != 0; // { dg-warning "value computed is not used" "" { target c++20 } } +} +#endif