[PATCH] D103938: Diagnose -Wunused-value based on CFG reachability

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D103938#3024938 <https://reviews.llvm.org/D103938#3024938>, @ychen wrote:

> 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, related code is `Sema::EmitCurrentDiagnostic`). 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

That will only help users who have newer versions of abseil. Also, I'm a bit worried this may be a code pattern used by more than just abseil.

The diagnostic text says that the comma operator has no effect, but this is a case where it does have a compile-time effect so the diagnostic sounds misleading. This suggests to me that we should silence the diagnostic in this case (because we want on-by-default diagnostics to be as close to free from false positives as possible). However, does silencing this cause significant issues with false negatives in other code examples?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103938/new/

https://reviews.llvm.org/D103938