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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
