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