On Wed, 1 May 2024, Jason Merrill wrote:
> On 4/12/24 13:22, Patrick Palka wrote:
> > On Fri, 12 Apr 2024, Jason Merrill wrote:
> >
> > > 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.
> >
> > Very interesting, thanks a lot.
>
> ...but I don't think my fixes are suitable for GCC 13, so would you apply your
> original patch to the 13 branch?
Will do
>
> Jason
>
>
