On Mon, 29 Jan 2024, Patrick Palka wrote:
> On Fri, 26 Jan 2024, Jason Merrill wrote:
>
> > On 1/26/24 17:11, Jason Merrill wrote:
> > > On 1/26/24 16:52, Jason Merrill wrote:
> > > > On 1/25/24 14:18, Patrick Palka wrote:
> > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
> > > > > OK for trunk/13? This isn't a very satisfactory fix, but at least
> > > > > it safely fixes these testcases I guess. Note that there's
> > > > > implementation disagreement about the second testcase, GCC always
> > > > > accepted it but Clang/MSVC/icc reject it.
> > > >
> > > > Because of trying to initialize int& from {c}; removing the extra braces
> > > > makes it work everywhore.
> > > >
> > > > https://eel.is/c++draft/dcl.init#list-3.10 says that we always generate
> > > > a
> > > > prvalue in this case, so perhaps we shouldn't recalculate if the
> > > > initializer is an init-list?
> > >
> > > ...but it seems bad to silently bind a const int& to a prvalue instead of
> > > directly to the reference returned by the operator, as clang does if we
> > > add
> > > const to the second testcase, so I think there's a defect in the standard
> > > here.
> >
> > Perhaps bullet 3.9 should change to "...its referenced type is
> > reference-related to E <ins>or scalar</ins>, ..."
> >
> > > Maybe for now also disable the maybe_valid heuristics in the case of an
> > > init-list?
> > >
> > > > The first testcase is special because it's a C-style cast; seems like
> > > > the
> > > > maybe_valid = false heuristics should be disabled if c_cast_p.
>
> Thanks a lot for the pointers. IIUC c_cast_p and LOOKUP_SHORTCUT_BAD_CONVS
> should already be mutually exclusive, since the latter is set only when
> computing argument conversions, so it shouldn't be necessary to check
> c_cast_p.
>
> I suppose we could disable the heuristic for init-lists, but after some
> digging I noticed that the heuristics were originally in same spot they
> are now until r5-601-gd02f620dc0bb3b moved them to get checked after
> the recursive recalculation case in reference_binding, returning a bad
> conversion instead of NULL. (Then in r13-1755-g68f37670eff0b872 I moved
> them back; IIRC that's why I felt confident that moving the checks was safe.)
> Thus we didn't always accept the second testcase, we only started doing so in
> GCC 5: https://godbolt.org/z/6nsEW14fh (sorry for missing this and saying we
> always accepted it)
>
> And indeed the current order of checks seems consistent with that of
> [dcl.init.ref]/5. So I wonder if we don't instead want to "complete"
> the NULL-to-bad-conversion adjustment in r5-601-gd02f620dc0bb3b and
> do:
>
> gcc/cp/ChangeLog:
>
> * call.cc (reference_binding): Set bad_p according to
> maybe_valid_p in the recursive case as well. Remove
> redundant gcc_assert.
>
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index 9de0d77c423..c4158b2af37 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -2033,8 +2033,8 @@ reference_binding (tree rto, tree rfrom, tree expr,
> bool c_cast_p, int flags,
> sflags, complain);
> if (!new_second)
> return bad_direct_conv ? bad_direct_conv : nullptr;
> + t->bad_p = !maybe_valid_p;
Oops, that should be |= not =.
> > Perhaps bullet 3.9 should change to "...its referenced type is
> > reference-related to E <ins>or scalar</ins>, ..."
> conv = merge_conversion_sequences (t, new_second);
> - gcc_assert (maybe_valid_p || conv->bad_p);
> return conv;
> }
> }
>
> This'd mean we'd go back to rejecting the second testcase (only the
> call, not the direct-init, interestingly enough), but that seems to be
In the second testcase, with the above fix initialize_reference silently
returns error_mark_node for the direct-init without issuing a
diagnostic, because in the error path convert_like doesn't find anything
wrong with the bad conversion. So more changes need to be made if we
want to set bad_p in the recursive case of reference_binding it seems;
dunno if that's the path we want to go down?
On the other hand, disabling the badness checks in certain cases seems
to be undesirable as well, since AFAICT their current position is
consistent with [dcl.init.ref]/5?
So I wonder if we should just go with the safest thing at this stage,
which would be the original patch that removes the problematic assert?
> the correct behavior anyway IIUC. The testsuite is otherwise happy
> with this change.
