On Mon, 29 Jan 2024, 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?
Ping. > > > the correct behavior anyway IIUC. The testsuite is otherwise happy > > with this change.