[PATCH] D103938: Diagnose -Wunused-value based on CFG reachability
Yuanfang Chen via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 27 09:21:57 PDT 2021
ychen added a comment.
In D103938#3018918 <https://reviews.llvm.org/D103938#3018918>, @ychen wrote:
> In D103938#3018588 <https://reviews.llvm.org/D103938#3018588>, @aaron.ballman wrote:
>
>> In D103938#3013503 <https://reviews.llvm.org/D103938#3013503>, @thakis wrote:
>>
>>> This flags this code from absl:
>>>
>>> template <typename ValueT, typename GenT,
>>> typename std::enable_if<std::is_integral<ValueT>::value, int>::type =
>>> (GenT{}, 0)>
>>> constexpr FlagDefaultArg DefaultArg(int) {
>>> return {FlagDefaultSrc(GenT{}.value), FlagDefaultKind::kOneWord};
>>> }
>>>
>>> (https://source.chromium.org/chromium/chromium/src/+/main:third_party/abseil-cpp/absl/flags/internal/flag.h;l=293?q=third_party%2Fabseil-cpp%2Fabsl%2Fflags%2Finternal%2Fflag.h)
>>>
>>> ../../third_party/abseil-cpp/absl/flags/internal/flag.h:293:16: warning: left operand of comma operator has no effect [-Wunused-value]
>>> (GenT{}, 0)>
>>> ^ ~~
>>> ../../third_party/abseil-cpp/absl/flags/internal/flag.h:293:16: warning: left operand of comma operator has no effect [-Wunused-value]
>>> (GenT{}, 0)>
>>> ^ ~~
>>>
>>> I guess it has a SFINAE effect there?
>>
>> Sorry for having missed this comment before when reviewing the code @thakis, thanks for pinging on it! That does look like a false positive assuming it's being used for SFINAE. @ychen, can you take a look (and revert if it looks like it'll take a while to resolve)?
>
> @thakis sorry for missing that. Reverted. I'll take a look.
Diagnoses are suppressed in SFINAE context by default (https://github.com/llvm/llvm-project/blob/3dbf27e762008757c0de7034c778d941bfeb0388/clang/include/clang/Basic/Diagnostic.td#L83). The test case triggered the warning because the substitution is successful for that overload candidate. When substitution failed, the warning is suppressed as expected. The test case I used is
#include<type_traits>
struct A {
int value;
};
template <typename ValueT, typename GenT,
typename std::enable_if<std::is_integral<ValueT>::value, int>::type =
(GenT{},0)>
constexpr int DefaultArg(int) {
return GenT{}.value;
}
template <typename ValueT, typename GenT>
constexpr int DefaultArg(double) {
return 1;
}
int foo() {
return DefaultArg<int,A>(0);
}
int bar() {
return DefaultArg<double,A>(0);
}
So I think the behavior is expected and it seems absl already patch that code with `(void)`. Is it ok with you to reland the patch @aaron.ballman @thakis ? Thanks
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103938/new/
https://reviews.llvm.org/D103938
More information about the cfe-commits
mailing list