This is an archive of the discontinued LLVM Phabricator instance.

Diagnose -Wunused-value based on CFG reachability
ClosedPublic

Authored by ychen on Jun 8 2021, 5:50 PM.

Details

Summary

While at it, add the diagnosis message "left operand of comma operator has no effect" (used by GCC) for comma operator.

This also makes Clang diagnose in the constant evaluation context which aligns with GCC/MSVC behavior. (https://godbolt.org/z/7zxb8Tx96)

Diff Detail

Event Timeline

ychen requested review of this revision.Jun 8 2021, 5:50 PM
ychen created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2021, 5:50 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rsmith added a comment.Jun 8 2021, 6:37 PM

This will diagnose unused values in unreachable code in constant-evaluated contexts; that doesn't seem quite right. For example, in:

void f() {
  new double[false ? (1, 2) : 3][false ? (1, 2) : 3];
}

... we'll diagnose that the 1 is unused in only one of the two array bounds, because one of them is a constant-evaluated context and the other is not.

I'd suggest we start by adding a separate DiagIfReachable function that does the same thing as DiagRuntimeBehavior except without the context check, with the intent to eventually refine it to check for reachability even in expressions for which we don't build a CFG (eg, in the initializer of a global or in a constant expression).

ychen updated this revision to Diff 351343.Jun 10 2021, 11:00 PM
  • Add DiagIfReachable and use it in Sema::DiagnoseUnusedExprResult.
  • Update tests.

This will diagnose unused values in unreachable code in constant-evaluated contexts; that doesn't seem quite right. For example, in:

void f() {
  new double[false ? (1, 2) : 3][false ? (1, 2) : 3];
}

... we'll diagnose that the 1 is unused in only one of the two array bounds, because one of them is a constant-evaluated context and the other is not.

I'd suggest we start by adding a separate DiagIfReachable function that does the same thing as DiagRuntimeBehavior except without the context check, with the intent to eventually refine it to check for reachability even in expressions for which we don't build a CFG (eg, in the initializer of a global or in a constant expression).

@rsmith Thanks for the pointer! Comments addressed. For the reachability check in contexts without CFG, it seems that some delayed processing is needed since constant evaluation happens later, however, I'm not sure about that.

I've given this some more thought, and I think it's only the "constant evaluated" check that we want to bypass in this case. It's common to use sizeof or decltype with a comma operator in order to put an expression in a SFINAE context, and I don't think we should warn on those cases. So I think we should still do the context check for DiagIfReachable, but only bail out for unevaluated contexts, not for constant-evaluated contexts. Does that seem reasonable?

clang/test/SemaCXX/warn-unused-value.cpp
148

Please add a FIXME saying that we shouldn't diagnose the unreachable constant expression here.

ychen updated this revision to Diff 355054.Jun 28 2021, 3:56 PM
  • Add test case for 'constexpr if'
ychen added a comment.Jun 28 2021, 3:57 PM

I've given this some more thought, and I think it's only the "constant evaluated" check that we want to bypass in this case. It's common to use sizeof or decltype with a comma operator in order to put an expression in a SFINAE context, and I don't think we should warn on those cases. So I think we should still do the context check for DiagIfReachable, but only bail out for unevaluated contexts, not for constant-evaluated contexts. Does that seem reasonable?

Yeah, it is reasonable and seems already in place (https://reviews.llvm.org/rG78ecb8737d97a038770af9f6a1ad1d7925c87eb7).

aaron.ballman added inline comments.Jul 15 2021, 7:44 AM
clang/include/clang/Sema/Sema.h
5105
clang/test/Sema/i-c-e.c
77–78

I think this diagnostic is kind of unfortunate because it increases my confusion -- the expression result is most assuredly *not* unused in these cases because it's used in the definition of the type.

clang/test/SemaCXX/warn-unused-value.cpp
145

This is another one that tripped me up due to the diagnostic wording.

ychen updated this revision to Diff 360514.Jul 21 2021, 10:36 AM
ychen marked an inline comment as done.
  • fix comment
clang/test/Sema/i-c-e.c
77–78

the expression result is most assuredly *not* unused in these cases because it's used in the definition of the type.

Do you mean "1" is used in the definition of the type? The warning is for "1" in this case. If I change 1 to any other number, the type of comma3 should not change, I think that means 1 is not used.

aaron.ballman added inline comments.Jul 22 2021, 5:25 AM
clang/test/Sema/i-c-e.c
77–78

Do you mean "1" is used in the definition of the type? The warning is for "1" in this case. If I change 1 to any other number, the type of comma3 should not change, I think that means 1 is not used.

I agree that the 1 is unused.

The trouble is that the diagnostic doesn't make that clear -- it just says the expression result is unused, which makes it sound like the entire comma expression result value is unused (which is not the case). I think the diagnostic should more clearly specify *what* is unused in this case, otherwise users may think the diagnostic is a false positive.

ychen updated this revision to Diff 371221.Sep 7 2021, 5:19 PM
  • Use more specific diagnoses "left operand of comma operator has no effect" when left operand of comma operator has no effect instead of "expression result is unused".
ychen added inline comments.Sep 7 2021, 5:21 PM
clang/test/Sema/i-c-e.c
77–78

Sounds good to me. I've used "left operand of comma operator has no effect" which is what GCC uses for this.

There were a few behavioral changes to tests that I had questions about. Also, can you add an additional test case that shows the behavior when the left operand of the comma expression is volatile (or do we already have that covered and I missed it)? e.g.,

int func() {
  volatile int *ip = (volatile int *)0xFEEDFACE;
  return (*ip, 1);
}

(In this case, we shouldn't diagnose that the left operand has no effect because reading a volatile variable is an operation with a side effect.)

clang/test/CodeCompletion/pragma-macro-token-caching.c
15

Why did we lose this diagnostic?

clang/test/Sema/exprs.c
19

Why did we lose this diagnostic (and the above comment about not changing the test)?

There were a few behavioral changes to tests that I had questions about. Also, can you add an additional test case that shows the behavior when the left operand of the comma expression is volatile (or do we already have that covered and I missed it)? e.g.,

int func() {
  volatile int *ip = (volatile int *)0xFEEDFACE;
  return (*ip, 1);
}

(In this case, we shouldn't diagnose that the left operand has no effect because reading a volatile variable is an operation with a side effect.)

It seems this is covered by svn157362.

clang/test/CodeCompletion/pragma-macro-token-caching.c
15

CFG analysis thinks it is unreachable, so it is not diagnosed.

clang/test/Sema/exprs.c
19

Because the behavior change here is that only reachable code is diagnosed. I should have kept the comment.

ychen updated this revision to Diff 373390.Sep 17 2021, 10:23 PM
  • Restore an useful comment.

Thanks for the review.

clang/test/Sema/exprs.c
19

I've put it back.

aaron.ballman accepted this revision.Sep 20 2021, 6:47 AM

There were a few behavioral changes to tests that I had questions about. Also, can you add an additional test case that shows the behavior when the left operand of the comma expression is volatile (or do we already have that covered and I missed it)? e.g.,

int func() {
  volatile int *ip = (volatile int *)0xFEEDFACE;
  return (*ip, 1);
}

(In this case, we shouldn't diagnose that the left operand has no effect because reading a volatile variable is an operation with a side effect.)

It seems this is covered by svn157362.

Thanks, LGTM!

clang/test/CodeCompletion/pragma-macro-token-caching.c
15

Oh, I see now -- the CFG thinks this is unreachable because of the previous error.

This revision is now accepted and ready to land.Sep 20 2021, 6:47 AM
ychen retitled this revision from Diagnose -Wunused-value in constant evaluation context to Diagnose -Wunused-value based on CFG reachability.Sep 20 2021, 10:42 AM
ychen edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Sep 20 2021, 10:44 AM
This revision was automatically updated to reflect the committed changes.

Hi @ychen,

A whole bunch of libcxx testcases fail for me when I run with this patch:

Failed Tests (90):
  libc++ :: libcxx/gdb/gdb_pretty_printer_test.sh.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/assign.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/copy.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/ctor_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/ctor_sseq.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/default.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/discard.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/io.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/seed_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/seed_sseq.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/values.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/eval_param.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.pconst/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.pconst/eval_param.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/eval_param.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.int/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.real/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/assign.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/copy.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/ctor_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/default.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/values.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/assign.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/copy.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/ctor_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/ctor_sseq.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/default.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/discard.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/io.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/seed_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/seed_sseq.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/values.pass.cpp
  libc++ :: std/numerics/rand/rand.predef/mt19937_64.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.cons/char_ptr_ctor.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.cons/default.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.cons/string_ctor.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.cons/ull_ctor.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.hash/bitset.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.hash/enabled_hash.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/any.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/count.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/flip_all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/flip_one.out_of_range.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/flip_one.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/index.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/index_const.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/left_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/left_shift_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/none.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/not_all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_and_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_eq_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_or_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_xor_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/reset_all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/reset_one.out_of_range.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/reset_one.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/right_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/right_shift_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/set_all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/set_one.out_of_range.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/set_one.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/size.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/test.out_of_range.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/test.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_string.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_ulong.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_and.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_not.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_or.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/stream_in.pass.cpp
  libc++ :: std/utilities/variant/variant.hash/hash.pass.cpp
  libc++ :: std/utilities/variant/variant.relops/relops.pass.cpp
  libc++ :: std/utilities/variant/variant.relops/relops_bool_conv.fail.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.assign/copy.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.assign/move.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.ctor/copy.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.ctor/move.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.status/index.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.status/valueless_by_exception.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp
  libc++ :: std/utilities/variant/variant.visit/visit.pass.cpp
  libc++ :: std/utilities/variant/variant.visit/visit_return_type.pass.cpp

The first one fails like

In file included from /repo/uabelho/master-github/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:18:
/repo/uabelho/master-github/llvm/build-all-builtins/include/c++/v1/bitset:671:98: error: implicit conversion from 'unsigned long' to 'const unsigned int' changes value from 288230376151711744 to 0 [-Werror,-Wconstant-conversion]
    static const unsigned __n_words = _Size == 0 ? 0 : (_Size - 1) / (sizeof(size_t) * CHAR_BIT) + 1;
                          ~~~~~~~~~                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
1 error generated.

error: command failed with exit status: 1

Can also be seen in build bots, e.g. here:
http://lab.llvm.org:8011/#/builders/119/builds/5612

gulfem added a subscriber: gulfem.Sep 21 2021, 9:17 AM

We also started seeing similar libc++ test failures in our Fuchsia builds:
https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8835548361443044001/+/u/clang/test/stdout

Failed Tests (89):
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/assign.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/copy.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/ctor_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/ctor_sseq.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/default.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/discard.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/io.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/seed_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/seed_sseq.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/values.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/eval_param.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.pconst/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.pconst/eval_param.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/eval_param.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.int/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.real/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/assign.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/copy.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/ctor_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/default.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/values.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/assign.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/copy.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/ctor_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/ctor_sseq.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/default.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/discard.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/io.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/seed_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/seed_sseq.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/values.pass.cpp
  libc++ :: std/numerics/rand/rand.predef/mt19937_64.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.cons/char_ptr_ctor.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.cons/default.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.cons/string_ctor.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.cons/ull_ctor.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.hash/bitset.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.hash/enabled_hash.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/any.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/count.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/flip_all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/flip_one.out_of_range.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/flip_one.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/index.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/index_const.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/left_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/left_shift_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/none.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/not_all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_and_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_eq_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_or_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_xor_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/reset_all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/reset_one.out_of_range.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/reset_one.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/right_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/right_shift_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/set_all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/set_one.out_of_range.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/set_one.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/size.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/test.out_of_range.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/test.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_string.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_ulong.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_and.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_not.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_or.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/stream_in.pass.cpp
  libc++ :: std/utilities/variant/variant.hash/hash.pass.cpp
  libc++ :: std/utilities/variant/variant.relops/relops.pass.cpp
  libc++ :: std/utilities/variant/variant.relops/relops_bool_conv.fail.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.assign/copy.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.assign/move.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.ctor/copy.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.ctor/move.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.status/index.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.status/valueless_by_exception.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp
  libc++ :: std/utilities/variant/variant.visit/visit.pass.cpp
  libc++ :: std/utilities/variant/variant.visit/visit_return_type.pass.cpp

Could you please revert it unless it could be fixed quickly?

We also started seeing similar libc++ test failures in our Fuchsia builds:
https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8835548361443044001/+/u/clang/test/stdout

Failed Tests (89):
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/assign.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/copy.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/ctor_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/ctor_sseq.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/default.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/discard.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/io.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/seed_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/seed_sseq.pass.cpp
  libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/values.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/eval_param.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.pconst/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.pconst/eval_param.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/eval_param.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.int/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.real/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/assign.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/copy.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/ctor_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/default.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/values.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/assign.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/copy.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/ctor_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/ctor_sseq.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/default.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/discard.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/eval.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/io.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/seed_result_type.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/seed_sseq.pass.cpp
  libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/values.pass.cpp
  libc++ :: std/numerics/rand/rand.predef/mt19937_64.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.cons/char_ptr_ctor.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.cons/default.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.cons/string_ctor.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.cons/ull_ctor.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.hash/bitset.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.hash/enabled_hash.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/any.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/count.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/flip_all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/flip_one.out_of_range.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/flip_one.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/index.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/index_const.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/left_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/left_shift_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/none.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/not_all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_and_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_eq_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_or_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_xor_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/reset_all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/reset_one.out_of_range.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/reset_one.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/right_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/right_shift_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/set_all.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/set_one.out_of_range.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/set_one.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/size.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/test.out_of_range.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/test.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_string.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_ulong.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_and.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_not.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_or.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/stream_in.pass.cpp
  libc++ :: std/utilities/variant/variant.hash/hash.pass.cpp
  libc++ :: std/utilities/variant/variant.relops/relops.pass.cpp
  libc++ :: std/utilities/variant/variant.relops/relops_bool_conv.fail.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.assign/copy.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.assign/move.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.ctor/copy.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.ctor/move.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.status/index.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.status/valueless_by_exception.pass.cpp
  libc++ :: std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp
  libc++ :: std/utilities/variant/variant.visit/visit.pass.cpp
  libc++ :: std/utilities/variant/variant.visit/visit_return_type.pass.cpp

Could you please revert it unless it could be fixed quickly?

I have reverted in 73a8bcd78921d38130fc42c90fd75d47b05b063d, thanks for letting us know about the test failures!

ychen added a comment.Sep 21 2021, 9:31 AM

@uabelho @gulfem Thanks for the remainder. I could not reproduce the issue with my Ubuntu box. Could you please attach a reproducer?

ychen added a comment.Sep 21 2021, 9:39 AM

@uabelho @gulfem Thanks for the remainder. I could not reproduce the issue with my Ubuntu box. Could you please attach a reproducer?

Never mind, I think I need "-Werror" to reproduce these.

thakis added a subscriber: thakis.Sep 21 2021, 1:28 PM

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?

ychen reopened this revision.Sep 21 2021, 5:51 PM

reopen to fix tests failure.

This revision is now accepted and ready to land.Sep 21 2021, 5:51 PM
ychen updated this revision to Diff 374078.Sep 21 2021, 5:51 PM
ychen edited the summary of this revision. (Show Details)
  • Check the scope correctly: first line of DiagIfReachable should be getCurFunctionOrMethodDecl(). And the FunctionScopes should be kept to deal with requires expression where FunctionScopes could be empty.
Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2021, 5:51 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
This revision now requires review to proceed.Sep 21 2021, 5:51 PM
ychen updated this revision to Diff 374080.Sep 21 2021, 5:56 PM
  • Remove accidentally included change.
ychen removed a reviewer: Restricted Project.Sep 21 2021, 5:56 PM
ychen removed a subscriber: libcxx-commits.
This revision is now accepted and ready to land.Sep 21 2021, 5:56 PM
ychen requested review of this revision.Sep 21 2021, 5:58 PM

This and 45c0ebe00efb should make -Werror build of libcxx tests pass.

MTC added a subscriber: MTC.Sep 21 2021, 8:35 PM
aaron.ballman accepted this revision.Sep 22 2021, 12:53 PM

The changes seem reasonable to me.

This revision is now accepted and ready to land.Sep 22 2021, 12:53 PM
This revision was landed with ongoing or failed builds.Sep 22 2021, 2:38 PM
This revision was automatically updated to reflect the committed changes.

This flags this code from absl:

Did you see this comment? Looks like this relanded, but it still warns there even though it has an effect there I think (?)

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

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.

ychen added a comment.EditedSep 27 2021, 9:21 AM

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

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

ychen reopened this revision.Sep 27 2021, 2:55 PM
This revision is now accepted and ready to land.Sep 27 2021, 2:55 PM
ychen updated this revision to Diff 375414.Sep 27 2021, 2:55 PM
  • Disable comma operator diagnose in SFINAE context.
ychen updated this revision to Diff 375422.Sep 27 2021, 3:27 PM
  • Add a test case.
ychen requested review of this revision.Sep 27 2021, 3:27 PM
aaron.ballman accepted this revision.Sep 28 2021, 4:14 AM

LGTM with some minor corrections in comments, thank you for the fix!

clang/lib/Sema/SemaStmt.cpp
381–383
clang/test/SemaCXX/warn-unused-value.cpp
154
This revision is now accepted and ready to land.Sep 28 2021, 4:14 AM
ychen updated this revision to Diff 375619.Sep 28 2021, 9:56 AM
ychen marked an inline comment as done.
  • Address comments.
This revision was landed with ongoing or failed builds.Sep 28 2021, 10:00 AM
This revision was automatically updated to reflect the committed changes.
xbolva00 added inline comments.
clang/include/clang/Sema/Sema.h
5108

Please adjust documentation, there is no "Statement" param.

ychen added inline comments.Oct 3 2021, 1:05 PM
clang/include/clang/Sema/Sema.h
5108

Fixed in a944f801cacdaa40. Thanks.