On Fri, Aug 10, 2018 at 8:59 AM, Marek Polacek <pola...@redhat.com> wrote:
> On Mon, Aug 06, 2018 at 12:02:31AM +1000, Jason Merrill wrote:
>> > OK -- see the patch below. Now, I'm not crazy about adding another bit
>> > to struct conversion, but reusing any of the other bits didn't seem
>> > safe/possible. Maybe it'll come in handy when dealing with this problem
>> > for function templates.
>>
>> Looks good.
>>
>> >> >> @@ -6669,9 +6669,12 @@ convert_nontype_argument (tree type, tree expr,
>> >> >> tsubst_flags_t complain)
>> >> >> /* C++17: A template-argument for a non-type
>> >> >> template-parameter shall
>> >> >> be a converted constant expression (8.20) of the type of
>> >> >> the
>> >> >> template-parameter. */
>> >> >> + int errs = errorcount;
>> >> >> expr = build_converted_constant_expr (type, expr, complain);
>> >> >> if (expr == error_mark_node)
>> >> >> - return error_mark_node;
>> >> >> + /* Make sure we return NULL_TREE only if we have really
>> >> >> issued
>> >> >> + an error, as described above. */
>> >> >> + return errorcount > errs ? NULL_TREE : error_mark_node;
>> >> >
>> >> > Is this still needed?
>> >>
>> >> Earlier you wrote,
>> >>
>> >> > Checking complain doesn't work because that doesn't
>> >> > say if we have really issued an error. If we have not, and we return
>> >> > NULL_TREE anyway, we hit this assert:
>> >> > 8517 gcc_assert (!(complain & tf_error) || seen_error ());
>> >>
>> >> If (complain & tf_error), we shouldn't see error_mark_node without an
>> >> error having been issued. Why is build_converted_constant_expr
>> >> returning error_mark_node without giving an error when (complain &
>> >> tf_error)?
>> >
>> > This can happen on invalid code; e.g. tests nontype13.C and crash87.C.
>> > What happens there is that we're trying to convert a METHOD_TYPE to bool,
>> > which doesn't work: in standard_conversion we go into the
>> > else if (tcode == BOOLEAN_TYPE)
>> > block, which returns NULL, implicit_conversion also then returns NULL, yet
>> > no error has been issued.
>>
>> Yes, that's normal. The problem is in build_converted_constant_expr:
>>
>> if (conv)
>> expr = convert_like (conv, expr, complain);
>> else
>> expr = error_mark_node;
>>
>> Here when setting expr to error_mark_node I forgot to give an error if
>> (complain & tf_error).
>
> Done. A few testcases needed adjusting but nothing surprising.
>
>> >> >> + /* If we're in a template and we know the constant value, we can
>> >> >> + warn. Otherwise wait for instantiation. */
>> >> >> + || (processing_template_decl && !TREE_CONSTANT (init)))
>> >> >
>> >> > I don't think we want this condition. If the value is non-constant
>> >> > but also not dependent, it'll never be constant, so we can go ahead
>> >> > and complain.
>> >
>> > (This to be investigated later.)
>>
>> Why later?
>
> So this was this wrong error:
> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg00159.html
> which was confusing. But I noticed it's because we instantiate 'size' twice:
> in compute_array_index_type:
> size = instantiate_non_dependent_expr_sfinae (size, complain);
> size = build_converted_constant_expr (size_type_node, size, complain);
> size = maybe_constant_value (size);
> and then in check_narrowing:
> init = fold_non_dependent_expr (init, complain);
>
> (in this case, size is a call to constexpr user-defined conversion).
>
> But check_narrowing now returns early if instantiation_dependent_expression_p,
> so I thought perhaps we could simply call maybe_constant_value which fixes
> this
> problem and doesn't regress anything. Does that seem like a sensible thing
> to do?
Hmm, that'll have problems if we pass an unfolded template expression
to check_narrowing, but probably we don't want to do that anyway. So
yes, it seems reasonable.
>> > --- gcc/cp/decl.c
>> > +++ gcc/cp/decl.c
>> > @@ -9581,7 +9581,7 @@ compute_array_index_type (tree name, tree size,
>> > tsubst_flags_t complain)
>> > {
>> > tree folded = cp_fully_fold (size);
>> > if (TREE_CODE (folded) == INTEGER_CST)
>> > - pedwarn (location_of (size), OPT_Wpedantic,
>> > + pedwarn (input_location, OPT_Wpedantic,
>>
>> It should work to use location_of (osize) here.
>
> I dropped this hunk altogether. Because location_of will use
> DECL_SOURCE_LOCATION for DECLs, the error message will point to the
> declaration
> itself, not the use. I don't really care either way.
We want the message to point to the use, which location_of (osize)
will provide, since it should still have a location wrapper around a
DECL.
> expr = build_converted_constant_expr (type, expr, complain);
> if (expr == error_mark_node)
> - return error_mark_node;
> + /* Make sure we return NULL_TREE only if we have really issued
> + an error, as described above. */
> + return (complain & tf_error) ? NULL_TREE : error_mark_node;
Now that you've fixed build_converted_constant_expr, we shouldn't need
this hunk.
Jason
