On 3/26/24 09:44, Patrick Palka wrote:
On Thu, 7 Mar 2024, Jason Merrill wrote:
On 1/29/24 17:42, Patrick Palka wrote:
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?
I still think the assert is correct, and the problem is that maybe_valid_p is
wrong; these cases turn out to be valid, so maybe_valid_p should be true.
I'm afraid then I don't know how we can statically identify these cases
without actually performing the conversion, in light of the recursion :/
Do you mind taking this PR? I don't feel well-versed enough with the
reference binding rules to tackle this adequately..
That ended up being a surprisingly deep dive, but I've now checked in
separate fixes for the two cases.
Jason
