This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Sun, Aug 12, 2018 at 2:13 AM, Marek Polacek <polacek@redhat.com> wrote:
> 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.

Hunh, that's strange.  Why isn't osize the unfolded expression?  Where
is the location wrapper getting stripped?

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

I suppose so.

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

Ah, OK.  That convention seems strange to me now, but that's not your
problem.  :)

Jason


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]