Page MenuHomePhabricator

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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–387
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
5120

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

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

Fixed in a944f801cacdaa40. Thanks.