On Tue, Jul 3, 2018 at 4:35 PM, Marek Polacek <pola...@redhat.com> wrote:
> On Tue, Jul 03, 2018 at 03:41:43PM -0400, Jason Merrill wrote:
>> On Tue, Jul 3, 2018 at 2:58 PM, Marek Polacek <pola...@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 <pola...@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 <pola...@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 <pola...@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 <pola...@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.
>
> Not quite sure if I follow. My very first attempt was to call check_narrowing
> in build_converted_constant_expr but that then doesn't detect narrowing in
> Wnarrowing8.C with user-defined conversion represented as a TARGET_EXPR that
> is not TREE_CONSTANT.
>
> I think I even tried putting check_narrowing to constexpr evaluation but
> that was I think too late in that the expression had already been evaluated
> and converted and the narrowing conversion was lost.
See my email after that one.
>> > @@ -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?
>
> With the current patch it is, to avoid redundant errors.
Likewise.
>> > + /* 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.
>
> That was because of constexpr-ex4.C with a VLA in a class template.
> check_narrowing gets a VAR_DECL 'a' and fold_non_dependent_expr-ing it
> fails with
> error: no matching function for call to ‘A::operator int(const A*)’
> I didn't think tweaking 'complain' was the way to go.
Hmm, that's surprising, I would expect that to work fine. I guess
this would be OK as a workaround, but I'd like to understand why that
would give a wrong error like that.
Jason
