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