| Message ID | 20201110002124.675939-1-polacek@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Improve static_assert diagnostic [PR97518] | expand |
On 11/9/20 7:21 PM, Marek Polacek wrote: > Currently, when a static_assert fails, we only say "static assertion failed". > It would be more useful if we could also print the expression that > evaluated to false; this is especially useful when the condition uses > template parameters. Consider the motivating example, in which we have > this line: > > static_assert(is_same<X, Y>::value); > > if this fails, the user has to play dirty games to get the compiler to > print the template arguments. With this patch, we say: > > static assertion failed due to requirement 'is_same<int*, int>::value' I'd rather avoid the word "requirement" here, to avoid confusion with Concepts. Maybe have the usual failed error, and if we're showing the expression, have a second inform to say e.g. "%qE evaluates to false"? Also, if we've narrowed the problem down to a particular subexpression, let's only print that one. > which I think is much better. However, always printing the condition that > evaluated to 'false' wouldn't be very useful: e.g. noexcept(fn) is > always parsed to true/false, so we would say "static assertion failed due > to requirement 'false'" which doesn't help. So I wound up only printing > the condition when it was instantiation-dependent, that is, we called > finish_static_assert from tsubst_expr. > > Moreover, this patch also improves the diagnostic when the condition > consists of a logical AND. Say you have something like this: > > static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); > > where fn4() evaluates to false and the other ones to true. Highlighting > the whole thing is not that helpful because it won't say which clause > evaluated to false. With the find_failing_clause tweak in this patch > we emit: > > error: static assertion failed > 6 | static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); > | ~~~^~ > > so you know right away what's going on. Unfortunately, when you combine > both things, that is, have an instantiation-dependent expr and && in > a static_assert, we can't yet quite point to the clause that failed. It > is because when we tsubstitute something like is_same<X, Y>::value, we > generate a VAR_DECL that doesn't have any location. It would be awesome > if we could wrap it with a location wrapper, but I didn't see anything > obvious. Hmm, I vaguely remember that at first we were using location wrappers less in templates, but I thought that was fixed. I don't see anything setting suppress_location_wrappers, and it looks like tsubst_copy_and_build should preserve a location wrapper. > Further tweak could be to print the failing clause of the condition if > possible, whether or not it was instantiation-dependent. If not instantiation-dependent, underlining the clause seems sufficient. > In passing, I've cleaned up some things: > * use iloc_sentinel when appropriate, > * it's nicer to call contextual_conv_bool instead of the rather verbose > perform_implicit_conversion_flags, > * no need to check for INTEGER_CST before calling integer_zerop. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > gcc/cp/ChangeLog: > > PR c++/97518 > * cp-tree.h (finish_static_assert): Adjust declaration. > * parser.c (cp_parser_static_assert): Pass false to > finish_static_assert. > * pt.c (tsubst_expr): Pass true to finish_static_assert. > * semantics.c (find_failing_clause_r): New function. > (find_failing_clause): New function. > (finish_static_assert): Add a bool parameter. Use > iloc_sentinel. Call contextual_conv_bool instead of > perform_implicit_conversion_flags. Don't check for INTEGER_CST before > calling integer_zerop. Call find_failing_clause and maybe use its > location. Print the original condition if SHOW_EXPR_P. > > gcc/testsuite/ChangeLog: > > PR c++/97518 > * g++.dg/diagnostic/pr87386.C: Adjust expected output. > * g++.dg/diagnostic/static_assert1.C: New test. > * g++.dg/diagnostic/static_assert2.C: New test. > > libcc1/ChangeLog: > > PR c++/97518 > * libcp1plugin.cc (plugin_add_static_assert): Pass false to > finish_static_assert. > > libstdc++-v3/ChangeLog: > > * testsuite/20_util/scoped_allocator/69293_neg.cc: Adjust dg-error. > * testsuite/20_util/uses_allocator/69293_neg.cc: Likewise. > * testsuite/20_util/uses_allocator/cons_neg.cc: Likewise. > * testsuite/26_numerics/random/pr60037-neg.cc: Likewise. > --- > gcc/cp/cp-tree.h | 2 +- > gcc/cp/parser.c | 3 +- > gcc/cp/pt.c | 6 +- > gcc/cp/semantics.c | 79 ++++++++++++++++--- > gcc/testsuite/g++.dg/diagnostic/pr87386.C | 2 +- > .../g++.dg/diagnostic/static_assert1.C | 20 +++++ > .../g++.dg/diagnostic/static_assert2.C | 68 ++++++++++++++++ > libcc1/libcp1plugin.cc | 2 +- > .../20_util/scoped_allocator/69293_neg.cc | 2 +- > .../20_util/uses_allocator/69293_neg.cc | 2 +- > .../20_util/uses_allocator/cons_neg.cc | 2 +- > .../26_numerics/random/pr60037-neg.cc | 4 +- > 12 files changed, 168 insertions(+), 24 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/diagnostic/static_assert1.C > create mode 100644 gcc/testsuite/g++.dg/diagnostic/static_assert2.C > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index b98d47a702f..230a1525c63 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -7234,7 +7234,7 @@ extern bool cxx_omp_create_clause_info (tree, tree, bool, bool, > bool, bool); > extern tree baselink_for_fns (tree); > extern void finish_static_assert (tree, tree, location_t, > - bool); > + bool, bool); > extern tree finish_decltype_type (tree, bool, tsubst_flags_t); > extern tree finish_trait_expr (location_t, enum cp_trait_kind, tree, tree); > extern tree build_lambda_expr (void); > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > index 4c819ea1c5d..1ef14279ff7 100644 > --- a/gcc/cp/parser.c > +++ b/gcc/cp/parser.c > @@ -14886,7 +14886,8 @@ cp_parser_static_assert(cp_parser *parser, bool member_p) > > /* Complete the static assertion, which may mean either processing > the static assert now or saving it for template instantiation. */ > - finish_static_assert (condition, message, assert_loc, member_p); > + finish_static_assert (condition, message, assert_loc, member_p, > + /*show_expr_p=*/false); > } > > /* Parse the expression in decltype ( expression ). */ > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > index a2655a0ff52..6ba114c9da3 100644 > --- a/gcc/cp/pt.c > +++ b/gcc/cp/pt.c > @@ -18492,8 +18492,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl, > tree condition; > > ++c_inhibit_evaluation_warnings; > - condition = > - tsubst_expr (STATIC_ASSERT_CONDITION (t), > + condition = > + tsubst_expr (STATIC_ASSERT_CONDITION (t), > args, > complain, in_decl, > /*integral_constant_expression_p=*/true); > @@ -18502,7 +18502,7 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl, > finish_static_assert (condition, > STATIC_ASSERT_MESSAGE (t), > STATIC_ASSERT_SOURCE_LOCATION (t), > - /*member_p=*/false); > + /*member_p=*/false, /*show_expr_p=*/true); > } > break; > > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c > index a550db69488..1cc5118748f 100644 > --- a/gcc/cp/semantics.c > +++ b/gcc/cp/semantics.c > @@ -9823,13 +9823,53 @@ init_cp_semantics (void) > { > } > > + > +/* If we have a condition in conjunctive normal form (CNF), find the first > + failing clause. In other words, given an expression like > + > + true && true && false && true && false > + > + return the first 'false'. EXPR is the expression. */ > + > +static tree > +find_failing_clause_r (tree expr) > +{ > + if (TREE_CODE (expr) == TRUTH_ANDIF_EXPR) > + { > + /* First check the left side... */ > + tree e = find_failing_clause_r (TREE_OPERAND (expr, 0)); > + if (e == NULL_TREE) > + /* ...if we didn't find a false clause, check the right side. */ > + e = find_failing_clause_r (TREE_OPERAND (expr, 1)); > + return e; > + } > + tree e = contextual_conv_bool (expr, tf_none); > + e = fold_non_dependent_expr (e, tf_none, /*manifestly_const_eval=*/true); > + if (integer_zerop (e)) > + /* This is the failing clause. */ > + return expr; > + return NULL_TREE; > +} > + > +/* Wrapper for find_failing_clause_r. */ > + > +static tree > +find_failing_clause (tree expr) > +{ > + if (TREE_CODE (expr) != TRUTH_ANDIF_EXPR) > + return NULL_TREE; > + return find_failing_clause_r (expr); > +} > + > /* Build a STATIC_ASSERT for a static assertion with the condition > CONDITION and the message text MESSAGE. LOCATION is the location > of the static assertion in the source code. When MEMBER_P, this > - static assertion is a member of a class. */ > + static assertion is a member of a class. If SHOW_EXPR_P is true, > + print the condition (because it was instantiation-dependent). */ > + > void > finish_static_assert (tree condition, tree message, location_t location, > - bool member_p) > + bool member_p, bool show_expr_p) > { > tsubst_flags_t complain = tf_warning_or_error; > > @@ -9867,8 +9907,7 @@ finish_static_assert (tree condition, tree message, location_t location, > tree orig_condition = condition; > > /* Fold the expression and convert it to a boolean value. */ > - condition = perform_implicit_conversion_flags (boolean_type_node, condition, > - complain, LOOKUP_NORMAL); > + condition = contextual_conv_bool (condition, complain); > condition = fold_non_dependent_expr (condition, complain, > /*manifestly_const_eval=*/true); > > @@ -9877,21 +9916,38 @@ finish_static_assert (tree condition, tree message, location_t location, > ; > else > { > - location_t saved_loc = input_location; > + iloc_sentinel ils (location); > > - input_location = location; > - if (TREE_CODE (condition) == INTEGER_CST > - && integer_zerop (condition)) > + if (integer_zerop (condition)) > { > int sz = TREE_INT_CST_LOW (TYPE_SIZE_UNIT > (TREE_TYPE (TREE_TYPE (message)))); > int len = TREE_STRING_LENGTH (message) / sz - 1; > + > + /* See if we can find which clause was failing (for logical AND). */ > + tree bad = find_failing_clause (orig_condition); > + /* If not, or its location is unusable, fall back to the previous > + location. */ > + location_t cloc = location; > + if (cp_expr_location (bad) != UNKNOWN_LOCATION) > + cloc = cp_expr_location (bad); > + > /* Report the error. */ > if (len == 0) > - error ("static assertion failed"); > + { > + if (show_expr_p) > + error_at (cloc, "static assertion failed due to " > + "requirement %qE", orig_condition); > + else > + error_at (cloc, "static assertion failed"); > + } > + else if (show_expr_p) > + error_at (cloc, "static assertion failed due to " > + "requirement %qE: %s", orig_condition, > + TREE_STRING_POINTER (message)); > else > - error ("static assertion failed: %s", > - TREE_STRING_POINTER (message)); > + error_at (cloc, "static assertion failed: %s", > + TREE_STRING_POINTER (message)); > > /* Actually explain the failure if this is a concept check or a > requires-expression. */ > @@ -9905,7 +9961,6 @@ finish_static_assert (tree condition, tree message, location_t location, > if (require_rvalue_constant_expression (condition)) > cxx_constant_value (condition); > } > - input_location = saved_loc; > } > } > > diff --git a/gcc/testsuite/g++.dg/diagnostic/pr87386.C b/gcc/testsuite/g++.dg/diagnostic/pr87386.C > index 85726af9f01..679a5177f64 100644 > --- a/gcc/testsuite/g++.dg/diagnostic/pr87386.C > +++ b/gcc/testsuite/g++.dg/diagnostic/pr87386.C > @@ -14,5 +14,5 @@ static_assert (foo::test<int>::value, "foo"); // { dg-error "static assertion f > static_assert (foo::test<int>::value && true, "bar"); // { dg-error "static assertion failed: bar" } > /* { dg-begin-multiline-output "" } > static_assert (foo::test<int>::value && true, "bar"); > - ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ > + ~~~~~~~~~~~~~~~~^~~~~ > { dg-end-multiline-output "" } */ > diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert1.C b/gcc/testsuite/g++.dg/diagnostic/static_assert1.C > new file mode 100644 > index 00000000000..5cfb66a5ad7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert1.C > @@ -0,0 +1,20 @@ > +// PR c++/97518 > +// { dg-do compile { target c++17 } } > + > +template <typename T, typename U> struct is_same { static constexpr bool value = false; }; > +template <typename T> struct is_same<T, T> { static constexpr bool value = true; }; > + > +template <typename T> using some_metafunction_t = T; > + > +template <typename T> > +void foo(T ) { > + using X = T*; > + using Y = some_metafunction_t<T>; > + > + static_assert(is_same<X, Y>::value); // { dg-error {static assertion failed due to requirement .is_same<int\*, int>::value.} } > + static_assert(is_same<X, Y>::value, "foo"); // { dg-error {static assertion failed due to requirement .is_same<int\*, int>::value.: foo} } > +} > + > +void bar() { > + foo(0); > +} > diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert2.C b/gcc/testsuite/g++.dg/diagnostic/static_assert2.C > new file mode 100644 > index 00000000000..542697f99de > --- /dev/null > +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert2.C > @@ -0,0 +1,68 @@ > +// PR c++/97518 > +// { dg-do compile { target c++11 } } > +// { dg-options "-fdiagnostics-show-caret" } > + > +constexpr bool yes () { return true; } > +constexpr bool no () { return false; } > +constexpr bool yay = true; > +constexpr bool nay = false; > + > +void > +bar () > +{ > + static_assert (true && true && no(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (true && true && no(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert (yay && nay, ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (yay && nay, ""); > + ^~~ > + { dg-end-multiline-output "" } */ > + static_assert (yes() && no(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (yes() && no(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert (no() && yes(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (no() && yes(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert (no() && no() && yes(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (no() && no() && yes(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert (yes() && yes() && yes () && no() && yes(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (yes() && yes() && yes () && no() && yes(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert (yes() && yes() && yes () && (no() && yes()), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (yes() && yes() && yes () && (no() && yes()), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert ((yes() && no()) && no(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert ((yes() && no()) && no(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert ((yes() && no()) && no(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert ((yes() && no()) && no(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert ((no() || no()) && yes(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert ((no() || no()) && yes(), ""); > + ~~~~~~^~~~~~~~ > + { dg-end-multiline-output "" } */ > + static_assert ((yes() || no()) && no(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert ((yes() || no()) && no(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > +} > diff --git a/libcc1/libcp1plugin.cc b/libcc1/libcp1plugin.cc > index bab2751a5ce..67a235f9095 100644 > --- a/libcc1/libcp1plugin.cc > +++ b/libcc1/libcp1plugin.cc > @@ -3642,7 +3642,7 @@ plugin_add_static_assert (cc1_plugin::connection *self, > > bool member_p = at_class_scope_p (); > > - finish_static_assert (condition, message, loc, member_p); > + finish_static_assert (condition, message, loc, member_p, false); > > return 1; > } > diff --git a/libstdc++-v3/testsuite/20_util/scoped_allocator/69293_neg.cc b/libstdc++-v3/testsuite/20_util/scoped_allocator/69293_neg.cc > index 6f3cd0a2521..d28a4330504 100644 > --- a/libstdc++-v3/testsuite/20_util/scoped_allocator/69293_neg.cc > +++ b/libstdc++-v3/testsuite/20_util/scoped_allocator/69293_neg.cc > @@ -46,7 +46,7 @@ test01() > scoped_alloc sa; > auto p = sa.allocate(1); > sa.construct(p); // this is required to be ill-formed > - // { dg-error "failed: .* uses_allocator is true" "" { target *-*-* } 0 } > + // { dg-error "failed due to requirement \[^\n\r\]*: .* uses_allocator is true" "" { target *-*-* } 0 } > } > > // Needed because of PR c++/92193 > diff --git a/libstdc++-v3/testsuite/20_util/uses_allocator/69293_neg.cc b/libstdc++-v3/testsuite/20_util/uses_allocator/69293_neg.cc > index 423ecc4d90e..b6d925edcd3 100644 > --- a/libstdc++-v3/testsuite/20_util/uses_allocator/69293_neg.cc > +++ b/libstdc++-v3/testsuite/20_util/uses_allocator/69293_neg.cc > @@ -44,6 +44,6 @@ test01() > { > alloc_type a; > std::tuple<X> t(std::allocator_arg, a); // this is required to be ill-formed > - // { dg-error "failed: .* uses_allocator is true" "" { target *-*-* } 0 } > + // { dg-error "failed due to requirement \[^\n\r\]*: .* uses_allocator is true" "" { target *-*-* } 0 } > // { dg-error "no matching function for call" "" { target c++2a } 0 } > } > diff --git a/libstdc++-v3/testsuite/20_util/uses_allocator/cons_neg.cc b/libstdc++-v3/testsuite/20_util/uses_allocator/cons_neg.cc > index c3ff05aa72a..20190feaa0b 100644 > --- a/libstdc++-v3/testsuite/20_util/uses_allocator/cons_neg.cc > +++ b/libstdc++-v3/testsuite/20_util/uses_allocator/cons_neg.cc > @@ -43,5 +43,5 @@ void test01() > > tuple<Type> t(allocator_arg, a, 1); > } > -// { dg-error "failed: .* uses_allocator is true" "" { target *-*-* } 0 } > +// { dg-error "failed due to requirement \[^\n\r\]*: .* uses_allocator is true" "" { target *-*-* } 0 } > // { dg-error "no matching function for call" "" { target c++2a } 0 } > diff --git a/libstdc++-v3/testsuite/26_numerics/random/pr60037-neg.cc b/libstdc++-v3/testsuite/26_numerics/random/pr60037-neg.cc > index ba252ef34fe..e2066cd59ee 100644 > --- a/libstdc++-v3/testsuite/26_numerics/random/pr60037-neg.cc > +++ b/libstdc++-v3/testsuite/26_numerics/random/pr60037-neg.cc > @@ -10,6 +10,6 @@ std::__detail::_Adaptor<std::mt19937, unsigned long> aurng(urng); > auto x = std::generate_canonical<std::size_t, > std::numeric_limits<std::size_t>::digits>(urng); > > -// { dg-error "static assertion failed: template argument must be a floating point type" "" { target *-*-* } 167 } > +// { dg-error "static assertion failed due to requirement \[^\n\r\]*: template argument must be a floating point type" "" { target *-*-* } 167 } > > -// { dg-error "static assertion failed: template argument must be a floating point type" "" { target *-*-* } 3346 } > +// { dg-error "static assertion failed due to requirement \[^\n\r\]*: template argument must be a floating point type" "" { target *-*-* } 3346 } > > base-commit: 04126e46eb2d829d7b4149d394b667e878912cc8 >
On Tue, Nov 10, 2020 at 11:32:04AM -0500, Jason Merrill wrote: > On 11/9/20 7:21 PM, Marek Polacek wrote: > > Currently, when a static_assert fails, we only say "static assertion failed". > > It would be more useful if we could also print the expression that > > evaluated to false; this is especially useful when the condition uses > > template parameters. Consider the motivating example, in which we have > > this line: > > > > static_assert(is_same<X, Y>::value); > > > > if this fails, the user has to play dirty games to get the compiler to > > print the template arguments. With this patch, we say: > > > > static assertion failed due to requirement 'is_same<int*, int>::value' > > I'd rather avoid the word "requirement" here, to avoid confusion with > Concepts. > > Maybe have the usual failed error, and if we're showing the expression, have > a second inform to say e.g. "%qE evaluates to false"? Works for me. > Also, if we've narrowed the problem down to a particular subexpression, > let's only print that one. Done. > > which I think is much better. However, always printing the condition that > > evaluated to 'false' wouldn't be very useful: e.g. noexcept(fn) is > > always parsed to true/false, so we would say "static assertion failed due > > to requirement 'false'" which doesn't help. So I wound up only printing > > the condition when it was instantiation-dependent, that is, we called > > finish_static_assert from tsubst_expr. > > > > Moreover, this patch also improves the diagnostic when the condition > > consists of a logical AND. Say you have something like this: > > > > static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); > > > > where fn4() evaluates to false and the other ones to true. Highlighting > > the whole thing is not that helpful because it won't say which clause > > evaluated to false. With the find_failing_clause tweak in this patch > > we emit: > > > > error: static assertion failed > > 6 | static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); > > | ~~~^~ > > > > so you know right away what's going on. Unfortunately, when you combine > > both things, that is, have an instantiation-dependent expr and && in > > a static_assert, we can't yet quite point to the clause that failed. It > > is because when we tsubstitute something like is_same<X, Y>::value, we > > generate a VAR_DECL that doesn't have any location. It would be awesome > > if we could wrap it with a location wrapper, but I didn't see anything > > obvious. > > Hmm, I vaguely remember that at first we were using location wrappers less > in templates, but I thought that was fixed. I don't see anything setting > suppress_location_wrappers, and it looks like tsubst_copy_and_build should > preserve a location wrapper. The problem here is that tsubst_qualified_id produces a VAR_DECL and for those CAN_HAVE_LOCATION_P is false. If the user writes (bool) is_same<X, Y>::value, this works well. Maybe we could create an artificial (bool) for clauses of the logical AND operator; decay_conversion in cp_build_binary_op will likely do it anyway, it just doesn't have any location: cp_build_binary_op takes an op_location_t, but that doesn't track locations of the operands. Maybe some hacks in tsubst_copy_and_build/TRUTH_AND_EXPR would help, I'm not sure. > > Further tweak could be to print the failing clause of the condition if > > possible, whether or not it was instantiation-dependent. > > If not instantiation-dependent, underlining the clause seems sufficient. Ack. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- Currently, when a static_assert fails, we only say "static assertion failed". It would be more useful if we could also print the expression that evaluated to false; this is especially useful when the condition uses template parameters. Consider the motivating example, in which we have this line: static_assert(is_same<X, Y>::value); if this fails, the user has to play dirty games to get the compiler to print the template arguments. With this patch, we say: error: static assertion failed note: 'is_same<int*, int>::value' evaluates to false which I think is much better. However, always printing the condition that evaluated to 'false' wouldn't be very useful: e.g. noexcept(fn) is always parsed to true/false, so we would say "'false' evaluates to false" which doesn't help. So I wound up only printing the condition when it was instantiation-dependent, that is, we called finish_static_assert from tsubst_expr. Moreover, this patch also improves the diagnostic when the condition consists of a logical AND. Say you have something like this: static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); where fn4() evaluates to false and the other ones to true. Highlighting the whole thing is not that helpful because it won't say which clause evaluated to false. With the find_failing_clause tweak in this patch we emit: error: static assertion failed 6 | static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); | ~~~^~ so you know right away what's going on. Unfortunately, when you combine both things, that is, have an instantiation-dependent expr and && in a static_assert, we can't yet quite point to the clause that failed. It is because when we tsubstitute something like is_same<X, Y>::value, we generate a VAR_DECL that doesn't have any location. It would be awesome if we could wrap it with a location wrapper, but I didn't see anything obvious. In passing, I've cleaned up some things: * use iloc_sentinel when appropriate, * it's nicer to call contextual_conv_bool instead of the rather verbose perform_implicit_conversion_flags, * no need to check for INTEGER_CST before calling integer_zerop. gcc/cp/ChangeLog: PR c++/97518 * cp-tree.h (finish_static_assert): Adjust declaration. * parser.c (cp_parser_static_assert): Pass false to finish_static_assert. * pt.c (tsubst_expr): Pass true to finish_static_assert. * semantics.c (find_failing_clause_r): New function. (find_failing_clause): New function. (finish_static_assert): Add a bool parameter. Use iloc_sentinel. Call contextual_conv_bool instead of perform_implicit_conversion_flags. Don't check for INTEGER_CST before calling integer_zerop. Call find_failing_clause and maybe use its location. Print the original condition or the failing clause if SHOW_EXPR_P. gcc/testsuite/ChangeLog: PR c++/97518 * g++.dg/diagnostic/pr87386.C: Adjust expected output. * g++.dg/diagnostic/static_assert1.C: New test. * g++.dg/diagnostic/static_assert2.C: New test. libcc1/ChangeLog: PR c++/97518 * libcp1plugin.cc (plugin_add_static_assert): Pass false to finish_static_assert. --- gcc/cp/cp-tree.h | 2 +- gcc/cp/parser.c | 3 +- gcc/cp/pt.c | 6 +- gcc/cp/semantics.c | 73 ++++++++++++++++--- gcc/testsuite/g++.dg/diagnostic/pr87386.C | 2 +- .../g++.dg/diagnostic/static_assert1.C | 30 ++++++++ .../g++.dg/diagnostic/static_assert2.C | 68 +++++++++++++++++ libcc1/libcp1plugin.cc | 2 +- 8 files changed, 167 insertions(+), 19 deletions(-) create mode 100644 gcc/testsuite/g++.dg/diagnostic/static_assert1.C create mode 100644 gcc/testsuite/g++.dg/diagnostic/static_assert2.C diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index b98d47a702f..230a1525c63 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7234,7 +7234,7 @@ extern bool cxx_omp_create_clause_info (tree, tree, bool, bool, bool, bool); extern tree baselink_for_fns (tree); extern void finish_static_assert (tree, tree, location_t, - bool); + bool, bool); extern tree finish_decltype_type (tree, bool, tsubst_flags_t); extern tree finish_trait_expr (location_t, enum cp_trait_kind, tree, tree); extern tree build_lambda_expr (void); diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 9c08c0e46a2..36322812310 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -14886,7 +14886,8 @@ cp_parser_static_assert(cp_parser *parser, bool member_p) /* Complete the static assertion, which may mean either processing the static assert now or saving it for template instantiation. */ - finish_static_assert (condition, message, assert_loc, member_p); + finish_static_assert (condition, message, assert_loc, member_p, + /*show_expr_p=*/false); } /* Parse the expression in decltype ( expression ). */ diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c index a2655a0ff52..6ba114c9da3 100644 --- a/gcc/cp/pt.c +++ b/gcc/cp/pt.c @@ -18492,8 +18492,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl, tree condition; ++c_inhibit_evaluation_warnings; - condition = - tsubst_expr (STATIC_ASSERT_CONDITION (t), + condition = + tsubst_expr (STATIC_ASSERT_CONDITION (t), args, complain, in_decl, /*integral_constant_expression_p=*/true); @@ -18502,7 +18502,7 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl, finish_static_assert (condition, STATIC_ASSERT_MESSAGE (t), STATIC_ASSERT_SOURCE_LOCATION (t), - /*member_p=*/false); + /*member_p=*/false, /*show_expr_p=*/true); } break; diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c index 33d715edaec..089c83e511f 100644 --- a/gcc/cp/semantics.c +++ b/gcc/cp/semantics.c @@ -9827,13 +9827,53 @@ init_cp_semantics (void) { } + +/* If we have a condition in conjunctive normal form (CNF), find the first + failing clause. In other words, given an expression like + + true && true && false && true && false + + return the first 'false'. EXPR is the expression. */ + +static tree +find_failing_clause_r (tree expr) +{ + if (TREE_CODE (expr) == TRUTH_ANDIF_EXPR) + { + /* First check the left side... */ + tree e = find_failing_clause_r (TREE_OPERAND (expr, 0)); + if (e == NULL_TREE) + /* ...if we didn't find a false clause, check the right side. */ + e = find_failing_clause_r (TREE_OPERAND (expr, 1)); + return e; + } + tree e = contextual_conv_bool (expr, tf_none); + e = fold_non_dependent_expr (e, tf_none, /*manifestly_const_eval=*/true); + if (integer_zerop (e)) + /* This is the failing clause. */ + return expr; + return NULL_TREE; +} + +/* Wrapper for find_failing_clause_r. */ + +static tree +find_failing_clause (tree expr) +{ + if (TREE_CODE (expr) != TRUTH_ANDIF_EXPR) + return NULL_TREE; + return find_failing_clause_r (expr); +} + /* Build a STATIC_ASSERT for a static assertion with the condition CONDITION and the message text MESSAGE. LOCATION is the location of the static assertion in the source code. When MEMBER_P, this - static assertion is a member of a class. */ + static assertion is a member of a class. If SHOW_EXPR_P is true, + print the condition (because it was instantiation-dependent). */ + void finish_static_assert (tree condition, tree message, location_t location, - bool member_p) + bool member_p, bool show_expr_p) { tsubst_flags_t complain = tf_warning_or_error; @@ -9871,8 +9911,7 @@ finish_static_assert (tree condition, tree message, location_t location, tree orig_condition = condition; /* Fold the expression and convert it to a boolean value. */ - condition = perform_implicit_conversion_flags (boolean_type_node, condition, - complain, LOOKUP_NORMAL); + condition = contextual_conv_bool (condition, complain); condition = fold_non_dependent_expr (condition, complain, /*manifestly_const_eval=*/true); @@ -9881,21 +9920,32 @@ finish_static_assert (tree condition, tree message, location_t location, ; else { - location_t saved_loc = input_location; + iloc_sentinel ils (location); - input_location = location; - if (TREE_CODE (condition) == INTEGER_CST - && integer_zerop (condition)) + if (integer_zerop (condition)) { int sz = TREE_INT_CST_LOW (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (message)))); int len = TREE_STRING_LENGTH (message) / sz - 1; + + /* See if we can find which clause was failing (for logical AND). */ + tree bad = find_failing_clause (orig_condition); + /* If not, or its location is unusable, fall back to the previous + location. */ + location_t cloc = location; + if (cp_expr_location (bad) != UNKNOWN_LOCATION) + cloc = cp_expr_location (bad); + /* Report the error. */ if (len == 0) - error ("static assertion failed"); + error_at (cloc, "static assertion failed"); else - error ("static assertion failed: %s", - TREE_STRING_POINTER (message)); + error_at (cloc, "static assertion failed: %s", + TREE_STRING_POINTER (message)); + if (show_expr_p) + inform (cloc, "%qE evaluates to false", + /* Nobody wants to see the artifical (bool) cast. */ + (bad ? tree_strip_nop_conversions (bad) : orig_condition)); /* Actually explain the failure if this is a concept check or a requires-expression. */ @@ -9909,7 +9959,6 @@ finish_static_assert (tree condition, tree message, location_t location, if (require_rvalue_constant_expression (condition)) cxx_constant_value (condition); } - input_location = saved_loc; } } diff --git a/gcc/testsuite/g++.dg/diagnostic/pr87386.C b/gcc/testsuite/g++.dg/diagnostic/pr87386.C index 85726af9f01..679a5177f64 100644 --- a/gcc/testsuite/g++.dg/diagnostic/pr87386.C +++ b/gcc/testsuite/g++.dg/diagnostic/pr87386.C @@ -14,5 +14,5 @@ static_assert (foo::test<int>::value, "foo"); // { dg-error "static assertion f static_assert (foo::test<int>::value && true, "bar"); // { dg-error "static assertion failed: bar" } /* { dg-begin-multiline-output "" } static_assert (foo::test<int>::value && true, "bar"); - ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ + ~~~~~~~~~~~~~~~~^~~~~ { dg-end-multiline-output "" } */ diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert1.C b/gcc/testsuite/g++.dg/diagnostic/static_assert1.C new file mode 100644 index 00000000000..fecf3cc51a0 --- /dev/null +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert1.C @@ -0,0 +1,30 @@ +// PR c++/97518 +// { dg-do compile { target c++17 } } + +template <typename T, typename U> struct is_same { static constexpr bool value = false; }; +template <typename T> struct is_same<T, T> { static constexpr bool value = true; }; + +template <typename T> using some_metafunction_t = T; + +template <typename T> +void foo(T ) { + using X = T*; + using Y = some_metafunction_t<T>; + + static_assert(is_same<X, Y>::value); // { dg-error "static assertion failed" } + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 } + static_assert(is_same<X, Y>::value, "foo"); // { dg-error "static assertion failed: foo" } + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 } + static_assert(is_same<X, X>::value && is_same<X, Y>::value); // { dg-error "static assertion failed" } + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 } + static_assert(is_same<X, Y>::value && is_same<X, X>::value); // { dg-error "static assertion failed" } + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 } + static_assert(is_same<X, X>::value + && is_same<Y, Y>::value + && is_same<X, Y>::value); // { dg-error "static assertion failed" } + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 } +} + +void bar() { + foo(0); +} diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert2.C b/gcc/testsuite/g++.dg/diagnostic/static_assert2.C new file mode 100644 index 00000000000..542697f99de --- /dev/null +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert2.C @@ -0,0 +1,68 @@ +// PR c++/97518 +// { dg-do compile { target c++11 } } +// { dg-options "-fdiagnostics-show-caret" } + +constexpr bool yes () { return true; } +constexpr bool no () { return false; } +constexpr bool yay = true; +constexpr bool nay = false; + +void +bar () +{ + static_assert (true && true && no(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (true && true && no(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert (yay && nay, ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (yay && nay, ""); + ^~~ + { dg-end-multiline-output "" } */ + static_assert (yes() && no(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (yes() && no(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert (no() && yes(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (no() && yes(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert (no() && no() && yes(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (no() && no() && yes(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert (yes() && yes() && yes () && no() && yes(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (yes() && yes() && yes () && no() && yes(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert (yes() && yes() && yes () && (no() && yes()), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (yes() && yes() && yes () && (no() && yes()), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert ((yes() && no()) && no(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert ((yes() && no()) && no(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert ((yes() && no()) && no(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert ((yes() && no()) && no(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert ((no() || no()) && yes(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert ((no() || no()) && yes(), ""); + ~~~~~~^~~~~~~~ + { dg-end-multiline-output "" } */ + static_assert ((yes() || no()) && no(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert ((yes() || no()) && no(), ""); + ~~^~ + { dg-end-multiline-output "" } */ +} diff --git a/libcc1/libcp1plugin.cc b/libcc1/libcp1plugin.cc index bab2751a5ce..67a235f9095 100644 --- a/libcc1/libcp1plugin.cc +++ b/libcc1/libcp1plugin.cc @@ -3642,7 +3642,7 @@ plugin_add_static_assert (cc1_plugin::connection *self, bool member_p = at_class_scope_p (); - finish_static_assert (condition, message, loc, member_p); + finish_static_assert (condition, message, loc, member_p, false); return 1; } base-commit: 831f24a778a016c6ce1ae739235e3f7e1f28ed8c
On 11/10/20 1:59 PM, Marek Polacek wrote: > On Tue, Nov 10, 2020 at 11:32:04AM -0500, Jason Merrill wrote: >> On 11/9/20 7:21 PM, Marek Polacek wrote: >>> Currently, when a static_assert fails, we only say "static assertion failed". >>> It would be more useful if we could also print the expression that >>> evaluated to false; this is especially useful when the condition uses >>> template parameters. Consider the motivating example, in which we have >>> this line: >>> >>> static_assert(is_same<X, Y>::value); >>> >>> if this fails, the user has to play dirty games to get the compiler to >>> print the template arguments. With this patch, we say: >>> >>> static assertion failed due to requirement 'is_same<int*, int>::value' >> >> I'd rather avoid the word "requirement" here, to avoid confusion with >> Concepts. >> >> Maybe have the usual failed error, and if we're showing the expression, have >> a second inform to say e.g. "%qE evaluates to false"? > > Works for me. > >> Also, if we've narrowed the problem down to a particular subexpression, >> let's only print that one. > > Done. > >>> which I think is much better. However, always printing the condition that >>> evaluated to 'false' wouldn't be very useful: e.g. noexcept(fn) is >>> always parsed to true/false, so we would say "static assertion failed due >>> to requirement 'false'" which doesn't help. So I wound up only printing >>> the condition when it was instantiation-dependent, that is, we called >>> finish_static_assert from tsubst_expr. >>> >>> Moreover, this patch also improves the diagnostic when the condition >>> consists of a logical AND. Say you have something like this: >>> >>> static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); >>> >>> where fn4() evaluates to false and the other ones to true. Highlighting >>> the whole thing is not that helpful because it won't say which clause >>> evaluated to false. With the find_failing_clause tweak in this patch >>> we emit: >>> >>> error: static assertion failed >>> 6 | static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); >>> | ~~~^~ >>> >>> so you know right away what's going on. Unfortunately, when you combine >>> both things, that is, have an instantiation-dependent expr and && in >>> a static_assert, we can't yet quite point to the clause that failed. It >>> is because when we tsubstitute something like is_same<X, Y>::value, we >>> generate a VAR_DECL that doesn't have any location. It would be awesome >>> if we could wrap it with a location wrapper, but I didn't see anything >>> obvious. >> >> Hmm, I vaguely remember that at first we were using location wrappers less >> in templates, but I thought that was fixed. I don't see anything setting >> suppress_location_wrappers, and it looks like tsubst_copy_and_build should >> preserve a location wrapper. > > The problem here is that tsubst_qualified_id produces a VAR_DECL and for those > CAN_HAVE_LOCATION_P is false. Ah, then perhaps tsubst_qualified_id should call maybe_wrap_with_location to preserve the location of the SCOPE_REF. > If the user writes (bool) is_same<X, Y>::value, > this works well. Maybe we could create an artificial (bool) for clauses of > the logical AND operator; decay_conversion in cp_build_binary_op will likely > do it anyway, it just doesn't have any location: cp_build_binary_op takes > an op_location_t, but that doesn't track locations of the operands. Maybe > some hacks in tsubst_copy_and_build/TRUTH_AND_EXPR would help, I'm not sure. > >>> Further tweak could be to print the failing clause of the condition if >>> possible, whether or not it was instantiation-dependent. >> >> If not instantiation-dependent, underlining the clause seems sufficient. > > Ack. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > -- >8 -- > Currently, when a static_assert fails, we only say "static assertion failed". > It would be more useful if we could also print the expression that > evaluated to false; this is especially useful when the condition uses > template parameters. Consider the motivating example, in which we have > this line: > > static_assert(is_same<X, Y>::value); > > if this fails, the user has to play dirty games to get the compiler to > print the template arguments. With this patch, we say: > > error: static assertion failed > note: 'is_same<int*, int>::value' evaluates to false > > which I think is much better. However, always printing the condition that > evaluated to 'false' wouldn't be very useful: e.g. noexcept(fn) is > always parsed to true/false, so we would say "'false' evaluates to false" > which doesn't help. So I wound up only printing the condition when it was > instantiation-dependent, that is, we called finish_static_assert from > tsubst_expr. > > Moreover, this patch also improves the diagnostic when the condition > consists of a logical AND. Say you have something like this: > > static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); > > where fn4() evaluates to false and the other ones to true. Highlighting > the whole thing is not that helpful because it won't say which clause > evaluated to false. With the find_failing_clause tweak in this patch > we emit: > > error: static assertion failed > 6 | static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); > | ~~~^~ > > so you know right away what's going on. Unfortunately, when you combine > both things, that is, have an instantiation-dependent expr and && in > a static_assert, we can't yet quite point to the clause that failed. It > is because when we tsubstitute something like is_same<X, Y>::value, we > generate a VAR_DECL that doesn't have any location. It would be awesome > if we could wrap it with a location wrapper, but I didn't see anything > obvious. > > In passing, I've cleaned up some things: > * use iloc_sentinel when appropriate, > * it's nicer to call contextual_conv_bool instead of the rather verbose > perform_implicit_conversion_flags, > * no need to check for INTEGER_CST before calling integer_zerop. > > gcc/cp/ChangeLog: > > PR c++/97518 > * cp-tree.h (finish_static_assert): Adjust declaration. > * parser.c (cp_parser_static_assert): Pass false to > finish_static_assert. > * pt.c (tsubst_expr): Pass true to finish_static_assert. > * semantics.c (find_failing_clause_r): New function. > (find_failing_clause): New function. > (finish_static_assert): Add a bool parameter. Use > iloc_sentinel. Call contextual_conv_bool instead of > perform_implicit_conversion_flags. Don't check for INTEGER_CST before > calling integer_zerop. Call find_failing_clause and maybe use its > location. Print the original condition or the failing clause if > SHOW_EXPR_P. > > gcc/testsuite/ChangeLog: > > PR c++/97518 > * g++.dg/diagnostic/pr87386.C: Adjust expected output. > * g++.dg/diagnostic/static_assert1.C: New test. > * g++.dg/diagnostic/static_assert2.C: New test. > > libcc1/ChangeLog: > > PR c++/97518 > * libcp1plugin.cc (plugin_add_static_assert): Pass false to > finish_static_assert. > --- > gcc/cp/cp-tree.h | 2 +- > gcc/cp/parser.c | 3 +- > gcc/cp/pt.c | 6 +- > gcc/cp/semantics.c | 73 ++++++++++++++++--- > gcc/testsuite/g++.dg/diagnostic/pr87386.C | 2 +- > .../g++.dg/diagnostic/static_assert1.C | 30 ++++++++ > .../g++.dg/diagnostic/static_assert2.C | 68 +++++++++++++++++ > libcc1/libcp1plugin.cc | 2 +- > 8 files changed, 167 insertions(+), 19 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/diagnostic/static_assert1.C > create mode 100644 gcc/testsuite/g++.dg/diagnostic/static_assert2.C > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index b98d47a702f..230a1525c63 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -7234,7 +7234,7 @@ extern bool cxx_omp_create_clause_info (tree, tree, bool, bool, > bool, bool); > extern tree baselink_for_fns (tree); > extern void finish_static_assert (tree, tree, location_t, > - bool); > + bool, bool); > extern tree finish_decltype_type (tree, bool, tsubst_flags_t); > extern tree finish_trait_expr (location_t, enum cp_trait_kind, tree, tree); > extern tree build_lambda_expr (void); > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > index 9c08c0e46a2..36322812310 100644 > --- a/gcc/cp/parser.c > +++ b/gcc/cp/parser.c > @@ -14886,7 +14886,8 @@ cp_parser_static_assert(cp_parser *parser, bool member_p) > > /* Complete the static assertion, which may mean either processing > the static assert now or saving it for template instantiation. */ > - finish_static_assert (condition, message, assert_loc, member_p); > + finish_static_assert (condition, message, assert_loc, member_p, > + /*show_expr_p=*/false); > } > > /* Parse the expression in decltype ( expression ). */ > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > index a2655a0ff52..6ba114c9da3 100644 > --- a/gcc/cp/pt.c > +++ b/gcc/cp/pt.c > @@ -18492,8 +18492,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl, > tree condition; > > ++c_inhibit_evaluation_warnings; > - condition = > - tsubst_expr (STATIC_ASSERT_CONDITION (t), > + condition = > + tsubst_expr (STATIC_ASSERT_CONDITION (t), > args, > complain, in_decl, > /*integral_constant_expression_p=*/true); > @@ -18502,7 +18502,7 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl, > finish_static_assert (condition, > STATIC_ASSERT_MESSAGE (t), > STATIC_ASSERT_SOURCE_LOCATION (t), > - /*member_p=*/false); > + /*member_p=*/false, /*show_expr_p=*/true); > } > break; > > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c > index 33d715edaec..089c83e511f 100644 > --- a/gcc/cp/semantics.c > +++ b/gcc/cp/semantics.c > @@ -9827,13 +9827,53 @@ init_cp_semantics (void) > { > } > > + > +/* If we have a condition in conjunctive normal form (CNF), find the first > + failing clause. In other words, given an expression like > + > + true && true && false && true && false > + > + return the first 'false'. EXPR is the expression. */ > + > +static tree > +find_failing_clause_r (tree expr) > +{ > + if (TREE_CODE (expr) == TRUTH_ANDIF_EXPR) > + { > + /* First check the left side... */ > + tree e = find_failing_clause_r (TREE_OPERAND (expr, 0)); > + if (e == NULL_TREE) > + /* ...if we didn't find a false clause, check the right side. */ > + e = find_failing_clause_r (TREE_OPERAND (expr, 1)); > + return e; > + } > + tree e = contextual_conv_bool (expr, tf_none); > + e = fold_non_dependent_expr (e, tf_none, /*manifestly_const_eval=*/true); > + if (integer_zerop (e)) > + /* This is the failing clause. */ > + return expr; > + return NULL_TREE; > +} > + > +/* Wrapper for find_failing_clause_r. */ > + > +static tree > +find_failing_clause (tree expr) > +{ > + if (TREE_CODE (expr) != TRUTH_ANDIF_EXPR) > + return NULL_TREE; > + return find_failing_clause_r (expr); > +} > + > /* Build a STATIC_ASSERT for a static assertion with the condition > CONDITION and the message text MESSAGE. LOCATION is the location > of the static assertion in the source code. When MEMBER_P, this > - static assertion is a member of a class. */ > + static assertion is a member of a class. If SHOW_EXPR_P is true, > + print the condition (because it was instantiation-dependent). */ > + > void > finish_static_assert (tree condition, tree message, location_t location, > - bool member_p) > + bool member_p, bool show_expr_p) > { > tsubst_flags_t complain = tf_warning_or_error; > > @@ -9871,8 +9911,7 @@ finish_static_assert (tree condition, tree message, location_t location, > tree orig_condition = condition; > > /* Fold the expression and convert it to a boolean value. */ > - condition = perform_implicit_conversion_flags (boolean_type_node, condition, > - complain, LOOKUP_NORMAL); > + condition = contextual_conv_bool (condition, complain); > condition = fold_non_dependent_expr (condition, complain, > /*manifestly_const_eval=*/true); > > @@ -9881,21 +9920,32 @@ finish_static_assert (tree condition, tree message, location_t location, > ; > else > { > - location_t saved_loc = input_location; > + iloc_sentinel ils (location); > > - input_location = location; > - if (TREE_CODE (condition) == INTEGER_CST > - && integer_zerop (condition)) > + if (integer_zerop (condition)) > { > int sz = TREE_INT_CST_LOW (TYPE_SIZE_UNIT > (TREE_TYPE (TREE_TYPE (message)))); > int len = TREE_STRING_LENGTH (message) / sz - 1; > + > + /* See if we can find which clause was failing (for logical AND). */ > + tree bad = find_failing_clause (orig_condition); > + /* If not, or its location is unusable, fall back to the previous > + location. */ > + location_t cloc = location; > + if (cp_expr_location (bad) != UNKNOWN_LOCATION) > + cloc = cp_expr_location (bad); > + > /* Report the error. */ > if (len == 0) > - error ("static assertion failed"); > + error_at (cloc, "static assertion failed"); > else > - error ("static assertion failed: %s", > - TREE_STRING_POINTER (message)); > + error_at (cloc, "static assertion failed: %s", > + TREE_STRING_POINTER (message)); > + if (show_expr_p) > + inform (cloc, "%qE evaluates to false", > + /* Nobody wants to see the artifical (bool) cast. */ "artificial" OK with that typo fixed. > + (bad ? tree_strip_nop_conversions (bad) : orig_condition)); > > /* Actually explain the failure if this is a concept check or a > requires-expression. */ > @@ -9909,7 +9959,6 @@ finish_static_assert (tree condition, tree message, location_t location, > if (require_rvalue_constant_expression (condition)) > cxx_constant_value (condition); > } > - input_location = saved_loc; > } > } > > diff --git a/gcc/testsuite/g++.dg/diagnostic/pr87386.C b/gcc/testsuite/g++.dg/diagnostic/pr87386.C > index 85726af9f01..679a5177f64 100644 > --- a/gcc/testsuite/g++.dg/diagnostic/pr87386.C > +++ b/gcc/testsuite/g++.dg/diagnostic/pr87386.C > @@ -14,5 +14,5 @@ static_assert (foo::test<int>::value, "foo"); // { dg-error "static assertion f > static_assert (foo::test<int>::value && true, "bar"); // { dg-error "static assertion failed: bar" } > /* { dg-begin-multiline-output "" } > static_assert (foo::test<int>::value && true, "bar"); > - ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ > + ~~~~~~~~~~~~~~~~^~~~~ > { dg-end-multiline-output "" } */ > diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert1.C b/gcc/testsuite/g++.dg/diagnostic/static_assert1.C > new file mode 100644 > index 00000000000..fecf3cc51a0 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert1.C > @@ -0,0 +1,30 @@ > +// PR c++/97518 > +// { dg-do compile { target c++17 } } > + > +template <typename T, typename U> struct is_same { static constexpr bool value = false; }; > +template <typename T> struct is_same<T, T> { static constexpr bool value = true; }; > + > +template <typename T> using some_metafunction_t = T; > + > +template <typename T> > +void foo(T ) { > + using X = T*; > + using Y = some_metafunction_t<T>; > + > + static_assert(is_same<X, Y>::value); // { dg-error "static assertion failed" } > + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 } > + static_assert(is_same<X, Y>::value, "foo"); // { dg-error "static assertion failed: foo" } > + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 } > + static_assert(is_same<X, X>::value && is_same<X, Y>::value); // { dg-error "static assertion failed" } > + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 } > + static_assert(is_same<X, Y>::value && is_same<X, X>::value); // { dg-error "static assertion failed" } > + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 } > + static_assert(is_same<X, X>::value > + && is_same<Y, Y>::value > + && is_same<X, Y>::value); // { dg-error "static assertion failed" } > + // { dg-message {.is_same<int\*, int>::value. evaluates to false} "" { target *-*-* } .-1 } > +} > + > +void bar() { > + foo(0); > +} > diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert2.C b/gcc/testsuite/g++.dg/diagnostic/static_assert2.C > new file mode 100644 > index 00000000000..542697f99de > --- /dev/null > +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert2.C > @@ -0,0 +1,68 @@ > +// PR c++/97518 > +// { dg-do compile { target c++11 } } > +// { dg-options "-fdiagnostics-show-caret" } > + > +constexpr bool yes () { return true; } > +constexpr bool no () { return false; } > +constexpr bool yay = true; > +constexpr bool nay = false; > + > +void > +bar () > +{ > + static_assert (true && true && no(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (true && true && no(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert (yay && nay, ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (yay && nay, ""); > + ^~~ > + { dg-end-multiline-output "" } */ > + static_assert (yes() && no(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (yes() && no(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert (no() && yes(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (no() && yes(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert (no() && no() && yes(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (no() && no() && yes(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert (yes() && yes() && yes () && no() && yes(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (yes() && yes() && yes () && no() && yes(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert (yes() && yes() && yes () && (no() && yes()), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert (yes() && yes() && yes () && (no() && yes()), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert ((yes() && no()) && no(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert ((yes() && no()) && no(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert ((yes() && no()) && no(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert ((yes() && no()) && no(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > + static_assert ((no() || no()) && yes(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert ((no() || no()) && yes(), ""); > + ~~~~~~^~~~~~~~ > + { dg-end-multiline-output "" } */ > + static_assert ((yes() || no()) && no(), ""); // { dg-error "static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert ((yes() || no()) && no(), ""); > + ~~^~ > + { dg-end-multiline-output "" } */ > +} > diff --git a/libcc1/libcp1plugin.cc b/libcc1/libcp1plugin.cc > index bab2751a5ce..67a235f9095 100644 > --- a/libcc1/libcp1plugin.cc > +++ b/libcc1/libcp1plugin.cc > @@ -3642,7 +3642,7 @@ plugin_add_static_assert (cc1_plugin::connection *self, > > bool member_p = at_class_scope_p (); > > - finish_static_assert (condition, message, loc, member_p); > + finish_static_assert (condition, message, loc, member_p, false); > > return 1; > } > > base-commit: 831f24a778a016c6ce1ae739235e3f7e1f28ed8c >
On Tue, Nov 10, 2020 at 02:15:56PM -0500, Jason Merrill wrote: > On 11/10/20 1:59 PM, Marek Polacek wrote: > > On Tue, Nov 10, 2020 at 11:32:04AM -0500, Jason Merrill wrote: > > > On 11/9/20 7:21 PM, Marek Polacek wrote: > > > > Currently, when a static_assert fails, we only say "static assertion failed". > > > > It would be more useful if we could also print the expression that > > > > evaluated to false; this is especially useful when the condition uses > > > > template parameters. Consider the motivating example, in which we have > > > > this line: > > > > > > > > static_assert(is_same<X, Y>::value); > > > > > > > > if this fails, the user has to play dirty games to get the compiler to > > > > print the template arguments. With this patch, we say: > > > > > > > > static assertion failed due to requirement 'is_same<int*, int>::value' > > > > > > I'd rather avoid the word "requirement" here, to avoid confusion with > > > Concepts. > > > > > > Maybe have the usual failed error, and if we're showing the expression, have > > > a second inform to say e.g. "%qE evaluates to false"? > > > > Works for me. > > > > > Also, if we've narrowed the problem down to a particular subexpression, > > > let's only print that one. > > > > Done. > > > > > > which I think is much better. However, always printing the condition that > > > > evaluated to 'false' wouldn't be very useful: e.g. noexcept(fn) is > > > > always parsed to true/false, so we would say "static assertion failed due > > > > to requirement 'false'" which doesn't help. So I wound up only printing > > > > the condition when it was instantiation-dependent, that is, we called > > > > finish_static_assert from tsubst_expr. > > > > > > > > Moreover, this patch also improves the diagnostic when the condition > > > > consists of a logical AND. Say you have something like this: > > > > > > > > static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); > > > > > > > > where fn4() evaluates to false and the other ones to true. Highlighting > > > > the whole thing is not that helpful because it won't say which clause > > > > evaluated to false. With the find_failing_clause tweak in this patch > > > > we emit: > > > > > > > > error: static assertion failed > > > > 6 | static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); > > > > | ~~~^~ > > > > > > > > so you know right away what's going on. Unfortunately, when you combine > > > > both things, that is, have an instantiation-dependent expr and && in > > > > a static_assert, we can't yet quite point to the clause that failed. It > > > > is because when we tsubstitute something like is_same<X, Y>::value, we > > > > generate a VAR_DECL that doesn't have any location. It would be awesome > > > > if we could wrap it with a location wrapper, but I didn't see anything > > > > obvious. > > > > > > Hmm, I vaguely remember that at first we were using location wrappers less > > > in templates, but I thought that was fixed. I don't see anything setting > > > suppress_location_wrappers, and it looks like tsubst_copy_and_build should > > > preserve a location wrapper. > > > > The problem here is that tsubst_qualified_id produces a VAR_DECL and for those > > CAN_HAVE_LOCATION_P is false. > > Ah, then perhaps tsubst_qualified_id should call maybe_wrap_with_location to > preserve the location of the SCOPE_REF. The SCOPE_REF's location is fine, we just throw it away and return a VAR_DECL. I think I'll have to extend op_location_t and pass something reasonable from tsubst_copy_and_build/TRUTH_AND_EXPR to cp_build_binary_op, and use it to set the operands' locations after decay_conversion :(. > > + /* See if we can find which clause was failing (for logical AND). */ > > + tree bad = find_failing_clause (orig_condition); > > + /* If not, or its location is unusable, fall back to the previous > > + location. */ > > + location_t cloc = location; > > + if (cp_expr_location (bad) != UNKNOWN_LOCATION) > > + cloc = cp_expr_location (bad); > > + > > /* Report the error. */ > > if (len == 0) > > - error ("static assertion failed"); > > + error_at (cloc, "static assertion failed"); > > else > > - error ("static assertion failed: %s", > > - TREE_STRING_POINTER (message)); > > + error_at (cloc, "static assertion failed: %s", > > + TREE_STRING_POINTER (message)); > > + if (show_expr_p) > > + inform (cloc, "%qE evaluates to false", > > + /* Nobody wants to see the artifical (bool) cast. */ > > "artificial" > > OK with that typo fixed. Thanks a lot. Marek
On 11/10/20 2:28 PM, Marek Polacek wrote: > On Tue, Nov 10, 2020 at 02:15:56PM -0500, Jason Merrill wrote: >> On 11/10/20 1:59 PM, Marek Polacek wrote: >>> On Tue, Nov 10, 2020 at 11:32:04AM -0500, Jason Merrill wrote: >>>> On 11/9/20 7:21 PM, Marek Polacek wrote: >>>>> Currently, when a static_assert fails, we only say "static assertion failed". >>>>> It would be more useful if we could also print the expression that >>>>> evaluated to false; this is especially useful when the condition uses >>>>> template parameters. Consider the motivating example, in which we have >>>>> this line: >>>>> >>>>> static_assert(is_same<X, Y>::value); >>>>> >>>>> if this fails, the user has to play dirty games to get the compiler to >>>>> print the template arguments. With this patch, we say: >>>>> >>>>> static assertion failed due to requirement 'is_same<int*, int>::value' >>>> >>>> I'd rather avoid the word "requirement" here, to avoid confusion with >>>> Concepts. >>>> >>>> Maybe have the usual failed error, and if we're showing the expression, have >>>> a second inform to say e.g. "%qE evaluates to false"? >>> >>> Works for me. >>> >>>> Also, if we've narrowed the problem down to a particular subexpression, >>>> let's only print that one. >>> >>> Done. >>> >>>>> which I think is much better. However, always printing the condition that >>>>> evaluated to 'false' wouldn't be very useful: e.g. noexcept(fn) is >>>>> always parsed to true/false, so we would say "static assertion failed due >>>>> to requirement 'false'" which doesn't help. So I wound up only printing >>>>> the condition when it was instantiation-dependent, that is, we called >>>>> finish_static_assert from tsubst_expr. >>>>> >>>>> Moreover, this patch also improves the diagnostic when the condition >>>>> consists of a logical AND. Say you have something like this: >>>>> >>>>> static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); >>>>> >>>>> where fn4() evaluates to false and the other ones to true. Highlighting >>>>> the whole thing is not that helpful because it won't say which clause >>>>> evaluated to false. With the find_failing_clause tweak in this patch >>>>> we emit: >>>>> >>>>> error: static assertion failed >>>>> 6 | static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); >>>>> | ~~~^~ >>>>> >>>>> so you know right away what's going on. Unfortunately, when you combine >>>>> both things, that is, have an instantiation-dependent expr and && in >>>>> a static_assert, we can't yet quite point to the clause that failed. It >>>>> is because when we tsubstitute something like is_same<X, Y>::value, we >>>>> generate a VAR_DECL that doesn't have any location. It would be awesome >>>>> if we could wrap it with a location wrapper, but I didn't see anything >>>>> obvious. >>>> >>>> Hmm, I vaguely remember that at first we were using location wrappers less >>>> in templates, but I thought that was fixed. I don't see anything setting >>>> suppress_location_wrappers, and it looks like tsubst_copy_and_build should >>>> preserve a location wrapper. >>> >>> The problem here is that tsubst_qualified_id produces a VAR_DECL and for those >>> CAN_HAVE_LOCATION_P is false. >> >> Ah, then perhaps tsubst_qualified_id should call maybe_wrap_with_location to >> preserve the location of the SCOPE_REF. > > The SCOPE_REF's location is fine, we just throw it away and return a VAR_DECL. Yes, I'm saying that tsubst_qualified_id should extract the location from the SCOPE_REF and pass it to maybe_wrap_with_location to give the VAR_DECL a location wrapper. Jason
On Tue, Nov 10, 2020 at 02:30:30PM -0500, Jason Merrill via Gcc-patches wrote: > On 11/10/20 2:28 PM, Marek Polacek wrote: > > On Tue, Nov 10, 2020 at 02:15:56PM -0500, Jason Merrill wrote: > > > On 11/10/20 1:59 PM, Marek Polacek wrote: > > > > On Tue, Nov 10, 2020 at 11:32:04AM -0500, Jason Merrill wrote: > > > > > On 11/9/20 7:21 PM, Marek Polacek wrote: > > > > > > Currently, when a static_assert fails, we only say "static assertion failed". > > > > > > It would be more useful if we could also print the expression that > > > > > > evaluated to false; this is especially useful when the condition uses > > > > > > template parameters. Consider the motivating example, in which we have > > > > > > this line: > > > > > > > > > > > > static_assert(is_same<X, Y>::value); > > > > > > > > > > > > if this fails, the user has to play dirty games to get the compiler to > > > > > > print the template arguments. With this patch, we say: > > > > > > > > > > > > static assertion failed due to requirement 'is_same<int*, int>::value' > > > > > > > > > > I'd rather avoid the word "requirement" here, to avoid confusion with > > > > > Concepts. > > > > > > > > > > Maybe have the usual failed error, and if we're showing the expression, have > > > > > a second inform to say e.g. "%qE evaluates to false"? > > > > > > > > Works for me. > > > > > > > > > Also, if we've narrowed the problem down to a particular subexpression, > > > > > let's only print that one. > > > > > > > > Done. > > > > > > > > > > which I think is much better. However, always printing the condition that > > > > > > evaluated to 'false' wouldn't be very useful: e.g. noexcept(fn) is > > > > > > always parsed to true/false, so we would say "static assertion failed due > > > > > > to requirement 'false'" which doesn't help. So I wound up only printing > > > > > > the condition when it was instantiation-dependent, that is, we called > > > > > > finish_static_assert from tsubst_expr. > > > > > > > > > > > > Moreover, this patch also improves the diagnostic when the condition > > > > > > consists of a logical AND. Say you have something like this: > > > > > > > > > > > > static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); > > > > > > > > > > > > where fn4() evaluates to false and the other ones to true. Highlighting > > > > > > the whole thing is not that helpful because it won't say which clause > > > > > > evaluated to false. With the find_failing_clause tweak in this patch > > > > > > we emit: > > > > > > > > > > > > error: static assertion failed > > > > > > 6 | static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); > > > > > > | ~~~^~ > > > > > > > > > > > > so you know right away what's going on. Unfortunately, when you combine > > > > > > both things, that is, have an instantiation-dependent expr and && in > > > > > > a static_assert, we can't yet quite point to the clause that failed. It > > > > > > is because when we tsubstitute something like is_same<X, Y>::value, we > > > > > > generate a VAR_DECL that doesn't have any location. It would be awesome > > > > > > if we could wrap it with a location wrapper, but I didn't see anything > > > > > > obvious. > > > > > > > > > > Hmm, I vaguely remember that at first we were using location wrappers less > > > > > in templates, but I thought that was fixed. I don't see anything setting > > > > > suppress_location_wrappers, and it looks like tsubst_copy_and_build should > > > > > preserve a location wrapper. > > > > > > > > The problem here is that tsubst_qualified_id produces a VAR_DECL and for those > > > > CAN_HAVE_LOCATION_P is false. > > > > > > Ah, then perhaps tsubst_qualified_id should call maybe_wrap_with_location to > > > preserve the location of the SCOPE_REF. > > > > The SCOPE_REF's location is fine, we just throw it away and return a VAR_DECL. > > Yes, I'm saying that tsubst_qualified_id should extract the location from > the SCOPE_REF and pass it to maybe_wrap_with_location to give the VAR_DECL a > location wrapper. Nevermind, I wasn't checking the return value of maybe_wrap_with_location... Maybe we should childproof it by marking it with WARN_UNUSED_RESULT. Anyway, this patch does the tsubst_qualified_id tweak. With that, the static_assert diagnostic with && is pretty spot on! Relatedly, don't create useless location wrappers for temporary variables. Since they are compiler-generated and ignored, nobody should be interested in their location in the source file. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- Retain the location when tsubstituting a qualified-id so that our static_assert diagnostic can benefit. Don't create useless location wrappers for temporary variables. gcc/ChangeLog: PR c++/97518 * tree.c (maybe_wrap_with_location): Don't add a location wrapper around an artificial and ignored decl. gcc/cp/ChangeLog: PR c++/97518 * pt.c (tsubst_qualified_id): Use EXPR_LOCATION of the qualified-id. Use it to maybe_wrap_with_location the final expression. gcc/testsuite/ChangeLog: PR c++/97518 * g++.dg/diagnostic/static_assert3.C: New test. --- gcc/cp/pt.c | 5 +-- .../g++.dg/diagnostic/static_assert3.C | 36 +++++++++++++++++++ gcc/tree.c | 4 +++ 3 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/diagnostic/static_assert3.C diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c index 6ba114c9da3..c592461c474 100644 --- a/gcc/cp/pt.c +++ b/gcc/cp/pt.c @@ -16214,7 +16214,7 @@ tsubst_qualified_id (tree qualified_id, tree args, tree name; bool is_template; tree template_args; - location_t loc = UNKNOWN_LOCATION; + location_t loc = EXPR_LOCATION (qualified_id); gcc_assert (TREE_CODE (qualified_id) == SCOPE_REF); @@ -16223,7 +16223,6 @@ tsubst_qualified_id (tree qualified_id, tree args, if (TREE_CODE (name) == TEMPLATE_ID_EXPR) { is_template = true; - loc = EXPR_LOCATION (name); template_args = TREE_OPERAND (name, 1); if (template_args) template_args = tsubst_template_args (template_args, args, @@ -16351,6 +16350,8 @@ tsubst_qualified_id (tree qualified_id, tree args, if (REF_PARENTHESIZED_P (qualified_id)) expr = force_paren_expr (expr); + expr = maybe_wrap_with_location (expr, loc); + return expr; } diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert3.C b/gcc/testsuite/g++.dg/diagnostic/static_assert3.C new file mode 100644 index 00000000000..5d363884508 --- /dev/null +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert3.C @@ -0,0 +1,36 @@ +// PR c++/97518 +// { dg-do compile { target c++17 } } +// { dg-options "-fdiagnostics-show-caret" } + +template <typename T, typename U> struct is_same { static constexpr bool value = false; }; +template <typename T> struct is_same<T, T> { static constexpr bool value = true; }; + +template <typename T, typename U> +void f(T, U) +{ + static_assert(is_same<T, T>::value && is_same<T, U>::value); // { dg-error "56:static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert(is_same<T, T>::value && is_same<T, U>::value); + ^~~~~ + { dg-end-multiline-output "" } */ +// { dg-message ".is_same<int, double>::value. evaluates to false" "" { target *-*-* } .-5 } + static_assert(is_same<U, T>::value && is_same<U, U>::value); // { dg-error "32:static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert(is_same<U, T>::value && is_same<U, U>::value); + ^~~~~ + { dg-end-multiline-output "" } */ +// { dg-message ".is_same<double, int>::value. evaluates to false" "" { target *-*-* } .-5 } + static_assert(is_same<U, U>::value + && is_same<U, T>::value // { dg-error "35:static assertion failed" } + && is_same<T, T>::value); +/* { dg-begin-multiline-output "" } + && is_same<U, T>::value + ^~~~~ + { dg-end-multiline-output "" } */ +// { dg-message ".is_same<double, int>::value. evaluates to false" "" { target *-*-* } .-6 } +} + +void g() +{ + f(0, 1.3); +} diff --git a/gcc/tree.c b/gcc/tree.c index 663f3ecfd12..34cc4116d66 100644 --- a/gcc/tree.c +++ b/gcc/tree.c @@ -15041,6 +15041,10 @@ maybe_wrap_with_location (tree expr, location_t loc) if (EXCEPTIONAL_CLASS_P (expr)) return expr; + /* Compiler-generated temporary variables don't need a wrapper. */ + if (DECL_P (expr) && DECL_ARTIFICIAL (expr) && DECL_IGNORED_P (expr)) + return expr; + /* If any auto_suppress_location_wrappers are active, don't create wrappers. */ if (suppress_location_wrappers > 0) base-commit: 9179d9da39cf6e72ab870647db893ef69316b103
On 11/10/20 8:13 PM, Marek Polacek wrote: > On Tue, Nov 10, 2020 at 02:30:30PM -0500, Jason Merrill via Gcc-patches wrote: >> On 11/10/20 2:28 PM, Marek Polacek wrote: >>> On Tue, Nov 10, 2020 at 02:15:56PM -0500, Jason Merrill wrote: >>>> On 11/10/20 1:59 PM, Marek Polacek wrote: >>>>> On Tue, Nov 10, 2020 at 11:32:04AM -0500, Jason Merrill wrote: >>>>>> On 11/9/20 7:21 PM, Marek Polacek wrote: >>>>>>> Currently, when a static_assert fails, we only say "static assertion failed". >>>>>>> It would be more useful if we could also print the expression that >>>>>>> evaluated to false; this is especially useful when the condition uses >>>>>>> template parameters. Consider the motivating example, in which we have >>>>>>> this line: >>>>>>> >>>>>>> static_assert(is_same<X, Y>::value); >>>>>>> >>>>>>> if this fails, the user has to play dirty games to get the compiler to >>>>>>> print the template arguments. With this patch, we say: >>>>>>> >>>>>>> static assertion failed due to requirement 'is_same<int*, int>::value' >>>>>> >>>>>> I'd rather avoid the word "requirement" here, to avoid confusion with >>>>>> Concepts. >>>>>> >>>>>> Maybe have the usual failed error, and if we're showing the expression, have >>>>>> a second inform to say e.g. "%qE evaluates to false"? >>>>> >>>>> Works for me. >>>>> >>>>>> Also, if we've narrowed the problem down to a particular subexpression, >>>>>> let's only print that one. >>>>> >>>>> Done. >>>>> >>>>>>> which I think is much better. However, always printing the condition that >>>>>>> evaluated to 'false' wouldn't be very useful: e.g. noexcept(fn) is >>>>>>> always parsed to true/false, so we would say "static assertion failed due >>>>>>> to requirement 'false'" which doesn't help. So I wound up only printing >>>>>>> the condition when it was instantiation-dependent, that is, we called >>>>>>> finish_static_assert from tsubst_expr. >>>>>>> >>>>>>> Moreover, this patch also improves the diagnostic when the condition >>>>>>> consists of a logical AND. Say you have something like this: >>>>>>> >>>>>>> static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); >>>>>>> >>>>>>> where fn4() evaluates to false and the other ones to true. Highlighting >>>>>>> the whole thing is not that helpful because it won't say which clause >>>>>>> evaluated to false. With the find_failing_clause tweak in this patch >>>>>>> we emit: >>>>>>> >>>>>>> error: static assertion failed >>>>>>> 6 | static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); >>>>>>> | ~~~^~ >>>>>>> >>>>>>> so you know right away what's going on. Unfortunately, when you combine >>>>>>> both things, that is, have an instantiation-dependent expr and && in >>>>>>> a static_assert, we can't yet quite point to the clause that failed. It >>>>>>> is because when we tsubstitute something like is_same<X, Y>::value, we >>>>>>> generate a VAR_DECL that doesn't have any location. It would be awesome >>>>>>> if we could wrap it with a location wrapper, but I didn't see anything >>>>>>> obvious. >>>>>> >>>>>> Hmm, I vaguely remember that at first we were using location wrappers less >>>>>> in templates, but I thought that was fixed. I don't see anything setting >>>>>> suppress_location_wrappers, and it looks like tsubst_copy_and_build should >>>>>> preserve a location wrapper. >>>>> >>>>> The problem here is that tsubst_qualified_id produces a VAR_DECL and for those >>>>> CAN_HAVE_LOCATION_P is false. >>>> >>>> Ah, then perhaps tsubst_qualified_id should call maybe_wrap_with_location to >>>> preserve the location of the SCOPE_REF. >>> >>> The SCOPE_REF's location is fine, we just throw it away and return a VAR_DECL. >> >> Yes, I'm saying that tsubst_qualified_id should extract the location from >> the SCOPE_REF and pass it to maybe_wrap_with_location to give the VAR_DECL a >> location wrapper. > > Nevermind, I wasn't checking the return value of maybe_wrap_with_location... > Maybe we should childproof it by marking it with WARN_UNUSED_RESULT. Sounds good. > Anyway, this patch does the tsubst_qualified_id tweak. With that, the > static_assert diagnostic with && is pretty spot on! > > Relatedly, don't create useless location wrappers for temporary variables. > Since they are compiler-generated and ignored, nobody should be interested > in their location in the source file. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? OK. > -- >8 -- > Retain the location when tsubstituting a qualified-id so that our > static_assert diagnostic can benefit. Don't create useless location > wrappers for temporary variables. > > gcc/ChangeLog: > > PR c++/97518 > * tree.c (maybe_wrap_with_location): Don't add a location > wrapper around an artificial and ignored decl. > > gcc/cp/ChangeLog: > > PR c++/97518 > * pt.c (tsubst_qualified_id): Use EXPR_LOCATION of the qualified-id. > Use it to maybe_wrap_with_location the final expression. > > gcc/testsuite/ChangeLog: > > PR c++/97518 > * g++.dg/diagnostic/static_assert3.C: New test. > --- > gcc/cp/pt.c | 5 +-- > .../g++.dg/diagnostic/static_assert3.C | 36 +++++++++++++++++++ > gcc/tree.c | 4 +++ > 3 files changed, 43 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/diagnostic/static_assert3.C > > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > index 6ba114c9da3..c592461c474 100644 > --- a/gcc/cp/pt.c > +++ b/gcc/cp/pt.c > @@ -16214,7 +16214,7 @@ tsubst_qualified_id (tree qualified_id, tree args, > tree name; > bool is_template; > tree template_args; > - location_t loc = UNKNOWN_LOCATION; > + location_t loc = EXPR_LOCATION (qualified_id); > > gcc_assert (TREE_CODE (qualified_id) == SCOPE_REF); > > @@ -16223,7 +16223,6 @@ tsubst_qualified_id (tree qualified_id, tree args, > if (TREE_CODE (name) == TEMPLATE_ID_EXPR) > { > is_template = true; > - loc = EXPR_LOCATION (name); > template_args = TREE_OPERAND (name, 1); > if (template_args) > template_args = tsubst_template_args (template_args, args, > @@ -16351,6 +16350,8 @@ tsubst_qualified_id (tree qualified_id, tree args, > if (REF_PARENTHESIZED_P (qualified_id)) > expr = force_paren_expr (expr); > > + expr = maybe_wrap_with_location (expr, loc); > + > return expr; > } > > diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert3.C b/gcc/testsuite/g++.dg/diagnostic/static_assert3.C > new file mode 100644 > index 00000000000..5d363884508 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert3.C > @@ -0,0 +1,36 @@ > +// PR c++/97518 > +// { dg-do compile { target c++17 } } > +// { dg-options "-fdiagnostics-show-caret" } > + > +template <typename T, typename U> struct is_same { static constexpr bool value = false; }; > +template <typename T> struct is_same<T, T> { static constexpr bool value = true; }; > + > +template <typename T, typename U> > +void f(T, U) > +{ > + static_assert(is_same<T, T>::value && is_same<T, U>::value); // { dg-error "56:static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert(is_same<T, T>::value && is_same<T, U>::value); > + ^~~~~ > + { dg-end-multiline-output "" } */ > +// { dg-message ".is_same<int, double>::value. evaluates to false" "" { target *-*-* } .-5 } > + static_assert(is_same<U, T>::value && is_same<U, U>::value); // { dg-error "32:static assertion failed" } > +/* { dg-begin-multiline-output "" } > + static_assert(is_same<U, T>::value && is_same<U, U>::value); > + ^~~~~ > + { dg-end-multiline-output "" } */ > +// { dg-message ".is_same<double, int>::value. evaluates to false" "" { target *-*-* } .-5 } > + static_assert(is_same<U, U>::value > + && is_same<U, T>::value // { dg-error "35:static assertion failed" } > + && is_same<T, T>::value); > +/* { dg-begin-multiline-output "" } > + && is_same<U, T>::value > + ^~~~~ > + { dg-end-multiline-output "" } */ > +// { dg-message ".is_same<double, int>::value. evaluates to false" "" { target *-*-* } .-6 } > +} > + > +void g() > +{ > + f(0, 1.3); > +} > diff --git a/gcc/tree.c b/gcc/tree.c > index 663f3ecfd12..34cc4116d66 100644 > --- a/gcc/tree.c > +++ b/gcc/tree.c > @@ -15041,6 +15041,10 @@ maybe_wrap_with_location (tree expr, location_t loc) > if (EXCEPTIONAL_CLASS_P (expr)) > return expr; > > + /* Compiler-generated temporary variables don't need a wrapper. */ > + if (DECL_P (expr) && DECL_ARTIFICIAL (expr) && DECL_IGNORED_P (expr)) > + return expr; > + > /* If any auto_suppress_location_wrappers are active, don't create > wrappers. */ > if (suppress_location_wrappers > 0) > > base-commit: 9179d9da39cf6e72ab870647db893ef69316b103 >
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index b98d47a702f..230a1525c63 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7234,7 +7234,7 @@ extern bool cxx_omp_create_clause_info (tree, tree, bool, bool, bool, bool); extern tree baselink_for_fns (tree); extern void finish_static_assert (tree, tree, location_t, - bool); + bool, bool); extern tree finish_decltype_type (tree, bool, tsubst_flags_t); extern tree finish_trait_expr (location_t, enum cp_trait_kind, tree, tree); extern tree build_lambda_expr (void); diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 4c819ea1c5d..1ef14279ff7 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -14886,7 +14886,8 @@ cp_parser_static_assert(cp_parser *parser, bool member_p) /* Complete the static assertion, which may mean either processing the static assert now or saving it for template instantiation. */ - finish_static_assert (condition, message, assert_loc, member_p); + finish_static_assert (condition, message, assert_loc, member_p, + /*show_expr_p=*/false); } /* Parse the expression in decltype ( expression ). */ diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c index a2655a0ff52..6ba114c9da3 100644 --- a/gcc/cp/pt.c +++ b/gcc/cp/pt.c @@ -18492,8 +18492,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl, tree condition; ++c_inhibit_evaluation_warnings; - condition = - tsubst_expr (STATIC_ASSERT_CONDITION (t), + condition = + tsubst_expr (STATIC_ASSERT_CONDITION (t), args, complain, in_decl, /*integral_constant_expression_p=*/true); @@ -18502,7 +18502,7 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl, finish_static_assert (condition, STATIC_ASSERT_MESSAGE (t), STATIC_ASSERT_SOURCE_LOCATION (t), - /*member_p=*/false); + /*member_p=*/false, /*show_expr_p=*/true); } break; diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c index a550db69488..1cc5118748f 100644 --- a/gcc/cp/semantics.c +++ b/gcc/cp/semantics.c @@ -9823,13 +9823,53 @@ init_cp_semantics (void) { } + +/* If we have a condition in conjunctive normal form (CNF), find the first + failing clause. In other words, given an expression like + + true && true && false && true && false + + return the first 'false'. EXPR is the expression. */ + +static tree +find_failing_clause_r (tree expr) +{ + if (TREE_CODE (expr) == TRUTH_ANDIF_EXPR) + { + /* First check the left side... */ + tree e = find_failing_clause_r (TREE_OPERAND (expr, 0)); + if (e == NULL_TREE) + /* ...if we didn't find a false clause, check the right side. */ + e = find_failing_clause_r (TREE_OPERAND (expr, 1)); + return e; + } + tree e = contextual_conv_bool (expr, tf_none); + e = fold_non_dependent_expr (e, tf_none, /*manifestly_const_eval=*/true); + if (integer_zerop (e)) + /* This is the failing clause. */ + return expr; + return NULL_TREE; +} + +/* Wrapper for find_failing_clause_r. */ + +static tree +find_failing_clause (tree expr) +{ + if (TREE_CODE (expr) != TRUTH_ANDIF_EXPR) + return NULL_TREE; + return find_failing_clause_r (expr); +} + /* Build a STATIC_ASSERT for a static assertion with the condition CONDITION and the message text MESSAGE. LOCATION is the location of the static assertion in the source code. When MEMBER_P, this - static assertion is a member of a class. */ + static assertion is a member of a class. If SHOW_EXPR_P is true, + print the condition (because it was instantiation-dependent). */ + void finish_static_assert (tree condition, tree message, location_t location, - bool member_p) + bool member_p, bool show_expr_p) { tsubst_flags_t complain = tf_warning_or_error; @@ -9867,8 +9907,7 @@ finish_static_assert (tree condition, tree message, location_t location, tree orig_condition = condition; /* Fold the expression and convert it to a boolean value. */ - condition = perform_implicit_conversion_flags (boolean_type_node, condition, - complain, LOOKUP_NORMAL); + condition = contextual_conv_bool (condition, complain); condition = fold_non_dependent_expr (condition, complain, /*manifestly_const_eval=*/true); @@ -9877,21 +9916,38 @@ finish_static_assert (tree condition, tree message, location_t location, ; else { - location_t saved_loc = input_location; + iloc_sentinel ils (location); - input_location = location; - if (TREE_CODE (condition) == INTEGER_CST - && integer_zerop (condition)) + if (integer_zerop (condition)) { int sz = TREE_INT_CST_LOW (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (message)))); int len = TREE_STRING_LENGTH (message) / sz - 1; + + /* See if we can find which clause was failing (for logical AND). */ + tree bad = find_failing_clause (orig_condition); + /* If not, or its location is unusable, fall back to the previous + location. */ + location_t cloc = location; + if (cp_expr_location (bad) != UNKNOWN_LOCATION) + cloc = cp_expr_location (bad); + /* Report the error. */ if (len == 0) - error ("static assertion failed"); + { + if (show_expr_p) + error_at (cloc, "static assertion failed due to " + "requirement %qE", orig_condition); + else + error_at (cloc, "static assertion failed"); + } + else if (show_expr_p) + error_at (cloc, "static assertion failed due to " + "requirement %qE: %s", orig_condition, + TREE_STRING_POINTER (message)); else - error ("static assertion failed: %s", - TREE_STRING_POINTER (message)); + error_at (cloc, "static assertion failed: %s", + TREE_STRING_POINTER (message)); /* Actually explain the failure if this is a concept check or a requires-expression. */ @@ -9905,7 +9961,6 @@ finish_static_assert (tree condition, tree message, location_t location, if (require_rvalue_constant_expression (condition)) cxx_constant_value (condition); } - input_location = saved_loc; } } diff --git a/gcc/testsuite/g++.dg/diagnostic/pr87386.C b/gcc/testsuite/g++.dg/diagnostic/pr87386.C index 85726af9f01..679a5177f64 100644 --- a/gcc/testsuite/g++.dg/diagnostic/pr87386.C +++ b/gcc/testsuite/g++.dg/diagnostic/pr87386.C @@ -14,5 +14,5 @@ static_assert (foo::test<int>::value, "foo"); // { dg-error "static assertion f static_assert (foo::test<int>::value && true, "bar"); // { dg-error "static assertion failed: bar" } /* { dg-begin-multiline-output "" } static_assert (foo::test<int>::value && true, "bar"); - ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ + ~~~~~~~~~~~~~~~~^~~~~ { dg-end-multiline-output "" } */ diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert1.C b/gcc/testsuite/g++.dg/diagnostic/static_assert1.C new file mode 100644 index 00000000000..5cfb66a5ad7 --- /dev/null +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert1.C @@ -0,0 +1,20 @@ +// PR c++/97518 +// { dg-do compile { target c++17 } } + +template <typename T, typename U> struct is_same { static constexpr bool value = false; }; +template <typename T> struct is_same<T, T> { static constexpr bool value = true; }; + +template <typename T> using some_metafunction_t = T; + +template <typename T> +void foo(T ) { + using X = T*; + using Y = some_metafunction_t<T>; + + static_assert(is_same<X, Y>::value); // { dg-error {static assertion failed due to requirement .is_same<int\*, int>::value.} } + static_assert(is_same<X, Y>::value, "foo"); // { dg-error {static assertion failed due to requirement .is_same<int\*, int>::value.: foo} } +} + +void bar() { + foo(0); +} diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert2.C b/gcc/testsuite/g++.dg/diagnostic/static_assert2.C new file mode 100644 index 00000000000..542697f99de --- /dev/null +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert2.C @@ -0,0 +1,68 @@ +// PR c++/97518 +// { dg-do compile { target c++11 } } +// { dg-options "-fdiagnostics-show-caret" } + +constexpr bool yes () { return true; } +constexpr bool no () { return false; } +constexpr bool yay = true; +constexpr bool nay = false; + +void +bar () +{ + static_assert (true && true && no(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (true && true && no(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert (yay && nay, ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (yay && nay, ""); + ^~~ + { dg-end-multiline-output "" } */ + static_assert (yes() && no(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (yes() && no(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert (no() && yes(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (no() && yes(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert (no() && no() && yes(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (no() && no() && yes(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert (yes() && yes() && yes () && no() && yes(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (yes() && yes() && yes () && no() && yes(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert (yes() && yes() && yes () && (no() && yes()), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert (yes() && yes() && yes () && (no() && yes()), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert ((yes() && no()) && no(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert ((yes() && no()) && no(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert ((yes() && no()) && no(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert ((yes() && no()) && no(), ""); + ~~^~ + { dg-end-multiline-output "" } */ + static_assert ((no() || no()) && yes(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert ((no() || no()) && yes(), ""); + ~~~~~~^~~~~~~~ + { dg-end-multiline-output "" } */ + static_assert ((yes() || no()) && no(), ""); // { dg-error "static assertion failed" } +/* { dg-begin-multiline-output "" } + static_assert ((yes() || no()) && no(), ""); + ~~^~ + { dg-end-multiline-output "" } */ +} diff --git a/libcc1/libcp1plugin.cc b/libcc1/libcp1plugin.cc index bab2751a5ce..67a235f9095 100644 --- a/libcc1/libcp1plugin.cc +++ b/libcc1/libcp1plugin.cc @@ -3642,7 +3642,7 @@ plugin_add_static_assert (cc1_plugin::connection *self, bool member_p = at_class_scope_p (); - finish_static_assert (condition, message, loc, member_p); + finish_static_assert (condition, message, loc, member_p, false); return 1; } diff --git a/libstdc++-v3/testsuite/20_util/scoped_allocator/69293_neg.cc b/libstdc++-v3/testsuite/20_util/scoped_allocator/69293_neg.cc index 6f3cd0a2521..d28a4330504 100644 --- a/libstdc++-v3/testsuite/20_util/scoped_allocator/69293_neg.cc +++ b/libstdc++-v3/testsuite/20_util/scoped_allocator/69293_neg.cc @@ -46,7 +46,7 @@ test01() scoped_alloc sa; auto p = sa.allocate(1); sa.construct(p); // this is required to be ill-formed - // { dg-error "failed: .* uses_allocator is true" "" { target *-*-* } 0 } + // { dg-error "failed due to requirement \[^\n\r\]*: .* uses_allocator is true" "" { target *-*-* } 0 } } // Needed because of PR c++/92193 diff --git a/libstdc++-v3/testsuite/20_util/uses_allocator/69293_neg.cc b/libstdc++-v3/testsuite/20_util/uses_allocator/69293_neg.cc index 423ecc4d90e..b6d925edcd3 100644 --- a/libstdc++-v3/testsuite/20_util/uses_allocator/69293_neg.cc +++ b/libstdc++-v3/testsuite/20_util/uses_allocator/69293_neg.cc @@ -44,6 +44,6 @@ test01() { alloc_type a; std::tuple<X> t(std::allocator_arg, a); // this is required to be ill-formed - // { dg-error "failed: .* uses_allocator is true" "" { target *-*-* } 0 } + // { dg-error "failed due to requirement \[^\n\r\]*: .* uses_allocator is true" "" { target *-*-* } 0 } // { dg-error "no matching function for call" "" { target c++2a } 0 } } diff --git a/libstdc++-v3/testsuite/20_util/uses_allocator/cons_neg.cc b/libstdc++-v3/testsuite/20_util/uses_allocator/cons_neg.cc index c3ff05aa72a..20190feaa0b 100644 --- a/libstdc++-v3/testsuite/20_util/uses_allocator/cons_neg.cc +++ b/libstdc++-v3/testsuite/20_util/uses_allocator/cons_neg.cc @@ -43,5 +43,5 @@ void test01() tuple<Type> t(allocator_arg, a, 1); } -// { dg-error "failed: .* uses_allocator is true" "" { target *-*-* } 0 } +// { dg-error "failed due to requirement \[^\n\r\]*: .* uses_allocator is true" "" { target *-*-* } 0 } // { dg-error "no matching function for call" "" { target c++2a } 0 } diff --git a/libstdc++-v3/testsuite/26_numerics/random/pr60037-neg.cc b/libstdc++-v3/testsuite/26_numerics/random/pr60037-neg.cc index ba252ef34fe..e2066cd59ee 100644 --- a/libstdc++-v3/testsuite/26_numerics/random/pr60037-neg.cc +++ b/libstdc++-v3/testsuite/26_numerics/random/pr60037-neg.cc @@ -10,6 +10,6 @@ std::__detail::_Adaptor<std::mt19937, unsigned long> aurng(urng); auto x = std::generate_canonical<std::size_t, std::numeric_limits<std::size_t>::digits>(urng); -// { dg-error "static assertion failed: template argument must be a floating point type" "" { target *-*-* } 167 } +// { dg-error "static assertion failed due to requirement \[^\n\r\]*: template argument must be a floating point type" "" { target *-*-* } 167 } -// { dg-error "static assertion failed: template argument must be a floating point type" "" { target *-*-* } 3346 } +// { dg-error "static assertion failed due to requirement \[^\n\r\]*: template argument must be a floating point type" "" { target *-*-* } 3346 }
Currently, when a static_assert fails, we only say "static assertion failed". It would be more useful if we could also print the expression that evaluated to false; this is especially useful when the condition uses template parameters. Consider the motivating example, in which we have this line: static_assert(is_same<X, Y>::value); if this fails, the user has to play dirty games to get the compiler to print the template arguments. With this patch, we say: static assertion failed due to requirement 'is_same<int*, int>::value' which I think is much better. However, always printing the condition that evaluated to 'false' wouldn't be very useful: e.g. noexcept(fn) is always parsed to true/false, so we would say "static assertion failed due to requirement 'false'" which doesn't help. So I wound up only printing the condition when it was instantiation-dependent, that is, we called finish_static_assert from tsubst_expr. Moreover, this patch also improves the diagnostic when the condition consists of a logical AND. Say you have something like this: static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); where fn4() evaluates to false and the other ones to true. Highlighting the whole thing is not that helpful because it won't say which clause evaluated to false. With the find_failing_clause tweak in this patch we emit: error: static assertion failed 6 | static_assert(fn1() && fn2() && fn3() && fn4() && fn5()); | ~~~^~ so you know right away what's going on. Unfortunately, when you combine both things, that is, have an instantiation-dependent expr and && in a static_assert, we can't yet quite point to the clause that failed. It is because when we tsubstitute something like is_same<X, Y>::value, we generate a VAR_DECL that doesn't have any location. It would be awesome if we could wrap it with a location wrapper, but I didn't see anything obvious. Further tweak could be to print the failing clause of the condition if possible, whether or not it was instantiation-dependent. In passing, I've cleaned up some things: * use iloc_sentinel when appropriate, * it's nicer to call contextual_conv_bool instead of the rather verbose perform_implicit_conversion_flags, * no need to check for INTEGER_CST before calling integer_zerop. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? gcc/cp/ChangeLog: PR c++/97518 * cp-tree.h (finish_static_assert): Adjust declaration. * parser.c (cp_parser_static_assert): Pass false to finish_static_assert. * pt.c (tsubst_expr): Pass true to finish_static_assert. * semantics.c (find_failing_clause_r): New function. (find_failing_clause): New function. (finish_static_assert): Add a bool parameter. Use iloc_sentinel. Call contextual_conv_bool instead of perform_implicit_conversion_flags. Don't check for INTEGER_CST before calling integer_zerop. Call find_failing_clause and maybe use its location. Print the original condition if SHOW_EXPR_P. gcc/testsuite/ChangeLog: PR c++/97518 * g++.dg/diagnostic/pr87386.C: Adjust expected output. * g++.dg/diagnostic/static_assert1.C: New test. * g++.dg/diagnostic/static_assert2.C: New test. libcc1/ChangeLog: PR c++/97518 * libcp1plugin.cc (plugin_add_static_assert): Pass false to finish_static_assert. libstdc++-v3/ChangeLog: * testsuite/20_util/scoped_allocator/69293_neg.cc: Adjust dg-error. * testsuite/20_util/uses_allocator/69293_neg.cc: Likewise. * testsuite/20_util/uses_allocator/cons_neg.cc: Likewise. * testsuite/26_numerics/random/pr60037-neg.cc: Likewise. --- gcc/cp/cp-tree.h | 2 +- gcc/cp/parser.c | 3 +- gcc/cp/pt.c | 6 +- gcc/cp/semantics.c | 79 ++++++++++++++++--- gcc/testsuite/g++.dg/diagnostic/pr87386.C | 2 +- .../g++.dg/diagnostic/static_assert1.C | 20 +++++ .../g++.dg/diagnostic/static_assert2.C | 68 ++++++++++++++++ libcc1/libcp1plugin.cc | 2 +- .../20_util/scoped_allocator/69293_neg.cc | 2 +- .../20_util/uses_allocator/69293_neg.cc | 2 +- .../20_util/uses_allocator/cons_neg.cc | 2 +- .../26_numerics/random/pr60037-neg.cc | 4 +- 12 files changed, 168 insertions(+), 24 deletions(-) create mode 100644 gcc/testsuite/g++.dg/diagnostic/static_assert1.C create mode 100644 gcc/testsuite/g++.dg/diagnostic/static_assert2.C base-commit: 04126e46eb2d829d7b4149d394b667e878912cc8