This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: C++ PATCH for c++/57891, narrowing conversions in non-type template arguments
- From: Jason Merrill <jason at redhat dot com>
- To: Marek Polacek <polacek at redhat dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 3 Jul 2018 16:27:33 -0400
- Subject: Re: C++ PATCH for c++/57891, narrowing conversions in non-type template arguments
- References: <20180627165343.GC4896@redhat.com> <CADzB+2nEwPa9mjqeLd4Zh_hOa93YTrp46SXcORx+y==fRs70SA@mail.gmail.com> <20180629195834.GB5927@redhat.com> <CADzB+2mMpGkwfY3mCKhwgFiJxmQ6wbU+WUQtvpXKJxQaprXn8Q@mail.gmail.com> <20180703185805.GH5927@redhat.com> <CADzB+2kg8nk2EmgcKqZ=S9HVUV_0LhOHKeOdoERcvnt2ZKpLLA@mail.gmail.com>
On Tue, Jul 3, 2018 at 3:41 PM, Jason Merrill <jason@redhat.com> wrote:
> On Tue, Jul 3, 2018 at 2:58 PM, Marek Polacek <polacek@redhat.com> wrote:
>> On Tue, Jul 03, 2018 at 12:40:51PM -0400, Jason Merrill wrote:
>>> On Fri, Jun 29, 2018 at 3:58 PM, Marek Polacek <polacek@redhat.com> wrote:
>>> > On Wed, Jun 27, 2018 at 07:35:15PM -0400, Jason Merrill wrote:
>>> >> On Wed, Jun 27, 2018 at 12:53 PM, Marek Polacek <polacek@redhat.com> wrote:
>>> >> > This PR complains about us accepting invalid code like
>>> >> >
>>> >> > template<unsigned int> struct A {};
>>> >> > A<-1> a;
>>> >> >
>>> >> > Where we should detect the narrowing: [temp.arg.nontype] says
>>> >> > "A template-argument for a non-type template-parameter shall be a converted
>>> >> > constant expression ([expr.const]) of the type of the template-parameter."
>>> >> > and a converted constant expression can contain only
>>> >> > - integral conversions other than narrowing conversions,
>>> >> > - [...]."
>>> >> > It spurred e.g.
>>> >> > <https://stackoverflow.com/questions/28184888/how-implicit-conversion-works-for-non-type-template-parameters>
>>> >> > and has >=3 dups so it has some visibility.
>>> >> >
>>> >> > I think build_converted_constant_expr needs to set check_narrowing.
>>> >> > check_narrowing also always mentions that it's in { } but that is no longer
>>> >> > true; in the future it will also apply to <=>. We'd probably have to add a new
>>> >> > flag to struct conversion if wanted to distinguish between these.
>>> >> >
>>> >> > This does not yet fix detecting narrowing in function templates (78244).
>>> >> >
>>> >> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
>>> >> >
>>> >> > 2018-06-27 Marek Polacek <polacek@redhat.com>
>>> >> >
>>> >> > PR c++/57891
>>> >> > * call.c (build_converted_constant_expr): Set check_narrowing.
>>> >> > * decl.c (compute_array_index_type): Add warning sentinel. Use
>>> >> > input_location.
>>> >> > * pt.c (convert_nontype_argument): Return NULL_TREE if any errors
>>> >> > were reported.
>>> >> > * typeck2.c (check_narrowing): Don't mention { } in diagnostic.
>>> >> >
>>> >> > * g++.dg/cpp0x/Wnarrowing6.C: New test.
>>> >> > * g++.dg/cpp0x/Wnarrowing7.C: New test.
>>> >> > * g++.dg/cpp0x/Wnarrowing8.C: New test.
>>> >> > * g++.dg/cpp0x/constexpr-data2.C: Add dg-error.
>>> >> > * g++.dg/init/new43.C: Adjust dg-error.
>>> >> > * g++.dg/other/fold1.C: Likewise.
>>> >> > * g++.dg/parse/array-size2.C: Likewise.
>>> >> > * g++.dg/other/vrp1.C: Add dg-error.
>>> >> > * g++.dg/template/char1.C: Likewise.
>>> >> > * g++.dg/ext/builtin12.C: Likewise.
>>> >> > * g++.dg/template/dependent-name3.C: Adjust dg-error.
>>> >> >
>>> >> > diff --git gcc/cp/call.c gcc/cp/call.c
>>> >> > index 209c1fd2f0e..956c7b149dc 100644
>>> >> > --- gcc/cp/call.c
>>> >> > +++ gcc/cp/call.c
>>> >> > @@ -4152,7 +4152,10 @@ build_converted_constant_expr (tree type, tree expr, tsubst_flags_t complain)
>>> >> > }
>>> >> >
>>> >> > if (conv)
>>> >> > - expr = convert_like (conv, expr, complain);
>>> >> > + {
>>> >> > + conv->check_narrowing = !processing_template_decl;
>>> >>
>>> >> Why !processing_template_decl? This needs a comment.
>>> >
>>> > Otherwise we'd warn for e.g.
>>> >
>>> > template<int N> struct S { char a[N]; };
>>> > S<1> s;
>>> >
>>> > where compute_array_index_type will try to convert the size of the array (which
>>> > is a template_parm_index of type int when parsing the template) to size_type.
>>> > So I guess I can say that we need to wait for instantiation?
>>>
>>> We certainly shouldn't give a narrowing diagnostic about a
>>> value-dependent expression. It probably makes sense to check that at
>>> the top of check_narrowing, with all the other early exit conditions.
>>> But if we do know the constant value in the template, it's good to
>>> complain then rather than wait for instantiation.
>>
>> Makes sense; how about this then? (Regtest/bootstrap running.)
>>
>> 2018-07-03 Marek Polacek <polacek@redhat.com>
>>
>> PR c++/57891
>> * call.c (build_converted_constant_expr): Set check_narrowing.
>> * decl.c (compute_array_index_type): Add warning sentinel. Use
>> input_location.
>> * pt.c (convert_nontype_argument): Return NULL_TREE if any errors
>> were reported.
>> * typeck2.c (check_narrowing): Don't warn for instantiation-dependent
>> expressions or non-constants in a template. Don't mention { } in
>> diagnostic.
>>
>> * g++.dg/cpp0x/Wnarrowing6.C: New test.
>> * g++.dg/cpp0x/Wnarrowing7.C: New test.
>> * g++.dg/cpp0x/Wnarrowing8.C: New test.
>> * g++.dg/cpp0x/Wnarrowing9.C: New test.
>> * g++.dg/cpp0x/Wnarrowing10.C: New test.
>> * g++.dg/cpp0x/constexpr-data2.C: Add dg-error.
>> * g++.dg/init/new43.C: Adjust dg-error.
>> * g++.dg/other/fold1.C: Likewise.
>> * g++.dg/parse/array-size2.C: Likewise.
>> * g++.dg/other/vrp1.C: Add dg-error.
>> * g++.dg/template/char1.C: Likewise.
>> * g++.dg/ext/builtin12.C: Likewise.
>> * g++.dg/template/dependent-name3.C: Adjust dg-error.
>>
>> diff --git gcc/cp/call.c gcc/cp/call.c
>> index 209c1fd2f0e..4fb0fa8774b 100644
>> --- gcc/cp/call.c
>> +++ gcc/cp/call.c
>> @@ -4152,7 +4152,10 @@ build_converted_constant_expr (tree type, tree expr, tsubst_flags_t complain)
>> }
>>
>> if (conv)
>> - expr = convert_like (conv, expr, complain);
>> + {
>> + conv->check_narrowing = true;
>> + expr = convert_like (conv, expr, complain);
>> + }
>> else
>> expr = error_mark_node;
>>
>> diff --git gcc/cp/decl.c gcc/cp/decl.c
>> index c04b9b7d457..8da63fa2aaa 100644
>> --- gcc/cp/decl.c
>> +++ gcc/cp/decl.c
>> @@ -9508,6 +9508,8 @@ compute_array_index_type (tree name, tree size, tsubst_flags_t complain)
>> else
>> {
>> size = instantiate_non_dependent_expr_sfinae (size, complain);
>> + /* Don't warn about narrowing for VLAs. */
>> + warning_sentinel s (warn_narrowing, !TREE_CONSTANT (osize));
>> size = build_converted_constant_expr (size_type_node, size, complain);
>
> Hmm, perhaps the underlying issue is that we only want
> build_converted_constant_expr to check for narrowing of constant
> values; if the value isn't constant, it isn't any kind of constant
> expression. So perhaps the checking needs to happen as part of
> constexpr evaluation.
On the other hand, check_narrowing itself forces constexpr evaluation,
so this could be handled all in the conversion code; we just need to
tell check_narrowing to allow non-constant values in that case.
>> @@ -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)?
>> + /* 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.
>
> Jason