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