Bug 90881 - -Wunused-value false positive in SFINAE context
Summary: -Wunused-value false positive in SFINAE context
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 9.1.1
: P3 normal
Target Milestone: ---
Assignee: Marek Polacek
URL:
Keywords: diagnostic
Depends on:
Blocks: Wunused
  Show dependency treegraph
 
Reported: 2019-06-13 23:29 UTC by Federico Kircheis
Modified: 2019-06-22 14:54 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-06-14 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Federico Kircheis 2019-06-13 23:29:32 UTC
Hello,

consider

----
#include <type_traits>

template <typename T, typename = void> struct status : std::false_type{};

template <typename T> struct status<T, decltype(T::member, void())> : std::true_type {};

struct s1{int member;};
struct s2{int _member;};

int main(){
	static_assert(status<s1>::value, "has member");
	static_assert(!status<s2>::value, "has no member");
}
----

When compiling with `-Wall` (or `-Wunused-value`), g++ generates following warning:


--
bug.cpp: In instantiation of ‘struct status<s1>’:
bug.cpp:11:26:   required from here
bug.cpp:5:58: warning: left operand of comma operator has no effect [-Wunused-value]
 template <typename T> struct status<T, decltype(T::member, void())> : std::true_type {};
                                                 ~~~~~~~~~^~~~~~~~
bug.cpp:5:58: warning: left operand of comma operator has no effect [-Wunused-value]
--


The warning is incorrect, as removing the left operand changes the behaviour of the program, thus it has effect:

----
#include <type_traits>

template <typename T, typename = void> struct status : std::false_type{};

template <typename T> struct status<T, decltype(void())> : std::true_type {}; // ! removed left operand

struct s1{int member;};
struct s2{int _member;};

int main(){
	static_assert(status<s1>::value, "has member");
	static_assert(!status<s2>::value, "has no member");
}
----

Does not compile:

--
bug.cpp: In function ‘int main()’:
bug.cpp:12:16: error: static assertion failed: has no member
  static_assert(!status<s2>::value, "has no member");
--

because `status` does not detect anymore if the member-variable `member` is present or not.
Comment 1 Jonathan Wakely 2019-06-14 00:02:25 UTC
Confirmed. We shouldn't give that warning in unevaluated contexts (decltype, sizeof, etc.) because unevaluated operands have no effects at all, with or without the comma operator.
Comment 2 Marek Polacek 2019-06-14 01:22:09 UTC
Patch:
https://gcc.gnu.org/ml/gcc-patches/2019-06/msg00810.html
Comment 3 Jonathan Wakely 2019-06-14 07:59:45 UTC
That was quick!
Comment 4 Federico Kircheis 2019-06-14 10:17:55 UTC
(In reply to Jonathan Wakely from comment #1)
> Confirmed. We shouldn't give that warning in unevaluated contexts (decltype,
> sizeof, etc.) because unevaluated operands have no effects at all, with or
> without the comma operator.

The compiler flag is about unused variables, not statements without effects.

I guess it would be nice for

----
int main(){
decltype(short(), int()) a = 1;
return a;
}
----

to have a warning that the left operand is unused (as of today), since here  SFINAE does not kick in:

--
<source>: In function 'int main()':
<source>:2:21: warning: left operand of comma operator has no effect [-Wunused-value]
    2 | decltype(short(), int()) a = 1;
      |                     ^
--

But maybe there are not enough use-cases to be worth to diagnostic it.
Comment 5 Marek Polacek 2019-06-14 11:09:12 UTC
(In reply to Federico Kircheis from comment #4)
> (In reply to Jonathan Wakely from comment #1)
> > Confirmed. We shouldn't give that warning in unevaluated contexts (decltype,
> > sizeof, etc.) because unevaluated operands have no effects at all, with or
> > without the comma operator.
> 
> The compiler flag is about unused variables, not statements without effects.
> 
> I guess it would be nice for
> 
> ----
> int main(){
> decltype(short(), int()) a = 1;
> return a;
> }
> ----
> 
> to have a warning that the left operand is unused (as of today), since here 
> SFINAE does not kick in:
> 
> --
> <source>: In function 'int main()':
> <source>:2:21: warning: left operand of comma operator has no effect
> [-Wunused-value]
>     2 | decltype(short(), int()) a = 1;
>       |                     ^
> --
> 
> But maybe there are not enough use-cases to be worth to diagnostic it.

With my patch, we wouldn't warn on this second testcase.  But clang++ doesn't warn, either.
Comment 6 Federico Kircheis 2019-06-14 12:44:31 UTC
> With my patch, we wouldn't warn on this second testcase.  But clang++
> doesn't warn, either.

Yes, I just wanted to point out that giving warning in unevaluated contexts has some benefits too.
I believe gcc is doing a better job than clang or msvc.

I actually thought about this bug, and would like to rephrase it.
It's not a false positive, it's an usability bug.



It's not problematic that gcc gives a warning, the text is problematic/misleading.

It says "left operand of comma operator has no effect", and as I showed this is wrong.

It should say "left operand of comma operator is not used" or something in that direction, since the warning should be about unused variables (`-Wunused-value`), and it would be correct.


As a programmer I can see more easily what gcc is trying to say, and I would be less tempted to remove the unused variable because gcc did not told me it was useless.


Now you might think, that this does not help me as programmer at all, since I would like to write warning-free code, and I might be tempted to remove the unused variable also in this situation.


Therefore, just add the hint (as actually documented), to cast the variable to void!
Exactly as on would normally do for other unused variables.
I did not think about it, because I payed too much attention at the error text, and not the cause of the warning.

This sample does compile, works as expected, and does not generate any warning.

----
#include <type_traits>

template <typename T, typename = void> struct status : std::false_type{};

template <typename T> struct status<T, decltype((void)T::member, void())> : std::true_type {};

struct s1{int member;};
struct s2{int _member;};

int main(){
	static_assert(status<s1>::value, "has member");
	static_assert(!status<s2>::value, "has no member");
}
----

What do you think?
Comment 7 Marek Polacek 2019-06-22 14:43:31 UTC
Author: mpolacek
Date: Sat Jun 22 14:43:00 2019
New Revision: 272585

URL: https://gcc.gnu.org/viewcvs?rev=272585&root=gcc&view=rev
Log:
	PR c++/90881 - bogus -Wunused-value in unevaluated context.
	* cvt.c (convert_to_void): Don't emit unused warnings in
	an unevaluated context.

	* g++.dg/cpp0x/Wunused-value1.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/cpp0x/Wunused-value1.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/cvt.c
    trunk/gcc/testsuite/ChangeLog
Comment 8 Marek Polacek 2019-06-22 14:48:04 UTC
Fixed.  The behavior is now asi outlined in Comment 5.
Comment 9 Federico Kircheis 2019-06-22 14:54:48 UTC
Hi,

did you consider my last comment (Comment 6)?

I find it unfortunate that gcc will not warn anymore about unused variables in some circumstances.

Maybe my example was not a good one, but I guess that a warning in `decltype(0,0)` would be useful if one intended `decltype(0.0)`.