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

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D103938#3025265 <https://reviews.llvm.org/D103938#3025265>, @aaron.ballman wrote:

> 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?

This makes great sense to me. IIUC, only constant-evaluation context could be in SFINAE context, so a check for this specific case should only affect the constant-evaluation context this patch is newly addressing. I'll update the patch and run some tests. Thanks for the quick reply.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103938