On Wed, Mar 21, 2018 at 4:14 PM, Marek Polacek <pola...@redhat.com> wrote:
> On Wed, Mar 21, 2018 at 02:55:36PM -0400, Jason Merrill wrote:
>> On Wed, Mar 21, 2018 at 2:37 PM, Marek Polacek <pola...@redhat.com> wrote:
>> > On Thu, Mar 15, 2018 at 08:55:59AM -0400, Jason Merrill wrote:
>> >> On Thu, Mar 15, 2018 at 7:49 AM, Marek Polacek <pola...@redhat.com> wrote:
>> >> > On Wed, Mar 14, 2018 at 02:06:36PM -0400, Jason Merrill wrote:
>> >> >> On Wed, Mar 14, 2018 at 11:59 AM, Marek Polacek <pola...@redhat.com>
>> >> >> wrote:
>> >> >> > cxx_constant_value doesn't understand template codes, and neither it
>> >> >> > understands OVERLOADs, so if we pass an OVERLOAD to it, we crash.
>> >> >> > Here
>> >> >> > instantiate_non_dependent_expr got an OVERLOAD, but since it calls
>> >> >> > is_nondependent_constant_expression which checks type_unknown_p, it
>> >> >> > left the
>> >> >> > expression as it was. We can't use
>> >> >> > is_nondependent_constant_expression in
>> >> >> > finish_if_stmt_cond because i_n_c_e checks is_constant_expression
>> >> >> > and that is
>> >> >> > not suitable here; we'd miss diagnostics. So I did the following; I
>> >> >> > think we
>> >> >> > should reject the testcase with an error.
>> >> >> >
>> >> >> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
>> >> >> >
>> >> >> > 2018-03-14 Marek Polacek <pola...@redhat.com>
>> >> >> >
>> >> >> > PR c++/84854
>> >> >> > * semantics.c (finish_if_stmt_cond): Give error if the
>> >> >> > condition
>> >> >> > is an overloaded function with no contextual type
>> >> >> > information.
>> >> >> >
>> >> >> > * g++.dg/cpp1z/constexpr-if15.C: New test.
>> >> >> >
>> >> >> > diff --git gcc/cp/semantics.c gcc/cp/semantics.c
>> >> >> > index fdf37bea770..a056e9445e9 100644
>> >> >> > --- gcc/cp/semantics.c
>> >> >> > +++ gcc/cp/semantics.c
>> >> >> > @@ -735,8 +735,16 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
>> >> >> > && require_constant_expression (cond)
>> >> >> > && !value_dependent_expression_p (cond))
>> >> >> > {
>> >> >> > - cond = instantiate_non_dependent_expr (cond);
>> >> >> > - cond = cxx_constant_value (cond, NULL_TREE);
>> >> >> > + if (type_unknown_p (cond))
>> >> >> > + {
>> >> >> > + cxx_incomplete_type_error (cond, TREE_TYPE (cond));
>> >> >> > + cond = error_mark_node;
>> >> >>
>> >> >> I think I'd prefer to skip this block when type_unknown_p, and leave
>> >> >> error handling up to the code shared with regular if.
>> >> >
>> >> > Like this?
>> >>
>> >> Yes, thanks.
>> >>
>> >> > It was my first version, but I thought we wanted the error;
>> >>
>> >> Getting an error is an improvement, but it should apply to
>> >> non-constexpr if as well, so checking in maybe_convert_cond would be
>> >> better. Actually, if we deal with unknown type there, we shouldn't
>> >> need this patch at all.
>> >>
>> >> ...except I also notice that since maybe_convert_cond doesn't do any
>> >> conversion in a template, we're trying to extract the constant value
>> >> without first converting to bool, which breaks this testcase:
>> >>
>> >> struct A
>> >> {
>> >> constexpr operator bool () { return true; }
>> >> int i;
>> >> };
>> >>
>> >> A a;
>> >>
>> >> template <class T> void f()
>> >> {
>> >> constexpr bool b = a; // works
>> >> if constexpr (a) { } // breaks
>> >> }
>> >>
>> >> int main()
>> >> {
>> >> f<int>();
>> >> }
>> >>
>> >> A simple fix would be to replace your type_unknown_p check here with a
>> >> comparison to boolean_type_node, so we wait until instantiation time
>> >> to require a constant value.
>> >
>> > Ok, that works.
>> >
>> > We should also make g++ accept the testcase with "static_assert(a)"
>> > instead of
>> > "if constexpr (a) { }" probably.
>>
>> > I guess the cxx_constant_value call in
>> > finish_static_assert should happen earlier?
>>
>> fold_non_dependent_expr should already have gotten the constant value,
>> the call to cxx_constant_value is just for giving an error.
>
> Oop, right.
>
>> The bug seems to be that is_nondependent_constant_expression doesn't
>> realize that the conversion to bool is OK because it uses a constexpr
>> member function.
>
> OK, I can look into this separately.
>
>> >> Better would be to actually do the conversion. Perhaps this could
>> >> share code (for converting and getting a constant value) with
>> >> finish_static_assert.
>> >
>> > But this I didn't manage to make to work. If I call
>> > perform_implicit_conversion_flags
>> > in maybe_convert_cond, I get
>> > error: cannot resolve overloaded function ‘foo’ based on conversion to
>> > type ‘bool’
>> > so I'm not sure how the conversion would help.
>>
>> That looks like a good diagnostic to me, what's the problem?
>
> Ugh, I got this wrong. That diagnostic is fine, because we can reject
> constexpr-if15.C, but with perform_implicit_conversion_flags we then
> can't evaluate constexpr-if16.C:
> error: the value of ‘a’ is not usable in a constant expression
>
>> > Anyway, here's at least the boolean_type_node version.
>> >
>> > Bootstrapped/regtested on x86_64-linux.
>> >
>> > 2018-03-21 Marek Polacek <pola...@redhat.com>
>> >
>> > PR c++/84854
>> > * semantics.c (finish_if_stmt_cond): Check if the type of the
>> > condition
>> > is boolean.
>>
>> OK.
>
> Thanks, will commit this part along with the testcases. Incrementally we
> should
> then a) add constexpr-if15.C diagnostic, b) accept constexpr-if16.C with
> static_assert, too.
I think we want to use instantiation_dependent_expression_p here, too.
Applying.
commit 255d33f7e10bd13e99b270e7ac34946e3a03dba1
Author: Jason Merrill <ja...@redhat.com>
Date: Mon Apr 2 17:58:41 2018 -0400
* semantics.c (finish_if_stmt_cond): Use instantiation_dependent_expression_p.
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index eef9e2f645d..ef243f6bf0a 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -733,7 +733,7 @@ finish_if_stmt_cond (tree cond, tree if_stmt)
if (IF_STMT_CONSTEXPR_P (if_stmt)
&& !type_dependent_expression_p (cond)
&& require_constant_expression (cond)
- && !value_dependent_expression_p (cond)
+ && !instantiation_dependent_expression_p (cond)
/* Wait until instantiation time, since only then COND has been
converted to bool. */
&& TREE_TYPE (cond) == boolean_type_node)
