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