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;
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
the correct behavior anyway IIUC. The testsuite is otherwise happy
with this change.
