Re: C++ PATCH for c++/57891, narrowing conversions in non-type template arguments

[Marek Polacek <polacek@redhat.com>]

On Sat, Aug 11, 2018 at 11:32:24PM +1200, Jason Merrill wrote:
> On Fri, Aug 10, 2018 at 8:59 AM, Marek Polacek <polacek@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.
 
Ok.

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

location_of (osize) is actually the same as location_of (size) so that didn't
change anything.  The code below uses input_location which is why I went with
it in the first place.  So, should I change this to input_location?

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

I think we should keep it; removing would violate the comment of the function
"If the conversion is unsuccessful, return NULL_TREE if we issued an error
message, or error_mark_node if we did not."
and it would also mean emitting redundant errors, which I'd like to avoid.

Seems like we only need to resolve the location thing now.  Thanks,

Marek