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
