On Tue, Jul 3, 2018 at 3:41 PM, Jason Merrill <ja...@redhat.com> 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.
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