Page MenuHomePhabricator

[libc++] [LWG2993] reference_wrapper<T> conversion from U&&

Authored by Quuxplusone on Dec 5 2020, 4:47 PM.



Implement the resolution of LWG2993. Replace a deleted constructor with a constructor that SFINAEs away in appropriate circumstances.
Also, now that the constructor is templated, we must have an explicit deduction guide to make CTAD work.

Some tests have been merged in from D40259. I discovered the existence of D40259 only after finishing the <__functional_base> patch itself, though; my impression is that my approach looks simpler than what @K-ballo did in that PR.

These two new tests fail before this patch, in C++11-through-C++20 modes:

The diffs in type_ctor.pass.cpp and copy_assign.pass.cpp are due to D40259, but have always been green (this patch doesn't affect their validity). I have no objection to removing those diffs, if requested, but I assumed they were in D40259 for some reason.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Dec 5 2020, 4:47 PM
Quuxplusone created this revision.
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 5 2020, 4:47 PM

Update Cxx2aStatusIssuesStatus.csv

Mordante added inline comments.

Can you add the version number in the last column?


Since the test requires C++17 you could use std::is_same_v and remove the empty strings from the static_assert.

Quuxplusone marked an inline comment as done.Dec 6 2020, 9:21 AM
Quuxplusone added inline comments.

A priori I'm not sure what the convention is here.

  • I've seen people adding "Clang 12.0" in some other files
  • but this file uses just "" consistently, e.g. on lines 43 and 44
  • as a code-hygiene thing, clearly the best thing to do is have the person who cuts the 12.0 release update all the empty strings to "12.0" in one single commit (or to "12" if that's the convention, or to "2020.17" if we move to a different numbering scheme, etc.) — this avoids forcing individual contributors to guess at the next release number before it's announced

So I'll happily and quickly do whatever, but I'd like @ldionne to tell me.


Will do!

Quuxplusone marked an inline comment as done.

Use C++17 is_same_v in one test.

Mordante added inline comments.Dec 6 2020, 9:36 AM

The column for the issues has been added last Friday. The papers had this column for a longer time (guess from day one) so I think we should use the same convention for the issues and papers:

"2993 <>__","reference_wrapper<T> conversion from T&&","Albuquerque","|Complete|","12.0"

I agree the other empty columns aren't great and maybe we should all put a version number there.

Just to understand, how is your approach simpler than the one in D40259?


Is there a difference between your approach, i.e. putting this in the ctor's body, and putting it into the initializer list?

Quuxplusone added inline comments.Dec 10 2020, 5:53 PM

This is how it's formally specified in LWG2993 — "Creates a variable r as if by T& r = std::forward<U>(u), then constructs a reference_wrapper object that stores a reference to r."
I'm not aware of any specific difference between what I wrote here and a D40259-style

    : __f_(_VSTD::addressof(__bind(static_cast<_Up&&>(__u))))
{ }

but then I haven't thought hard about it. Basically I don't see any advantage to diverging from the exact "as-if-by" wording. Going our own way here can potentially make us wrong, but it can't make us any more right. :)

Just to understand, how is your approach simpler than the one in D40259?

One fewer template parameter; exploit some recently added libc++ helpers like __is_same_uncvref; static_cast<_Up&&> versus std::forward<_Up>... I guess that's all. The implementation technique is basically the same. The lengthy comment threads on D40259 made me think the implementation was longer than it really was. :) There's some obsolete stuff in those comments too, like references to __void_t when the patch no longer uses __void_t.

Rebase on main; @ldionne ping!

Quuxplusone marked 3 inline comments as done.Wed, Feb 3, 4:06 PM
ldionne accepted this revision.Tue, Feb 9, 11:41 AM

Hmm, yeah, it looks like D40259 was never committed back in 2017. The work duplication is unfortunate, but let's move forward with this since this LGTM and CI is passing.


I hadn't thought about that, but it seems like we could:

  1. Fill all missing version numbers (in the worse case, if doing archeology is too hard/time consuming, just put "13.0" there).
  1. Once we have all the versions, we can have a policy that when you implement something, you just mark it as "Complete", and then the LLVM release is filled in when we cut a release. I agree with @Quuxplusone that this would simplify the life for contributors and also remove the need for people to rebase just to change a version number when something is committed around a release branch (like right now, where I'm often requesting that people change versions from 12.0 to 13.0 since those patches won't make it into 12.0).
This revision is now accepted and ready to land.Tue, Feb 9, 11:41 AM