From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3267 invoked by alias); 13 Aug 2018 10:14:46 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 3251 invoked by uid 89); 13 Aug 2018 10:14:45 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=complain, sensible, crazy X-HELO: mail-oi0-f67.google.com Received: from mail-oi0-f67.google.com (HELO mail-oi0-f67.google.com) (209.85.218.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 13 Aug 2018 10:14:43 +0000 Received: by mail-oi0-f67.google.com with SMTP id n21-v6so26304893oig.3 for ; Mon, 13 Aug 2018 03:14:43 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ac9:531d:0:0:0:0:0 with HTTP; Mon, 13 Aug 2018 03:14:21 -0700 (PDT) In-Reply-To: <20180811141350.GL4317@redhat.com> References: <20180629195834.GB5927@redhat.com> <20180703185805.GH5927@redhat.com> <20180723204912.GH29486@redhat.com> <20180809205940.GJ4317@redhat.com> <20180811141350.GL4317@redhat.com> From: Jason Merrill Date: Mon, 13 Aug 2018 10:14:00 -0000 Message-ID: Subject: Re: C++ PATCH for c++/57891, narrowing conversions in non-type template arguments To: Marek Polacek Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-08/txt/msg00721.txt.bz2 On Sun, Aug 12, 2018 at 2:13 AM, Marek Polacek 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 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