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
