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.
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.
>> 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?
> 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.
> (finish_static_assert): Remove redundant variable.
But not this hunk; I like to be able to use the name "complain" even
when it isn't a parameter.
Jason
