C++ Standard Library Request: std::forward

[Jeremy Roman]

I'd like to permit using std::forward from <utility> for perfect forwarding of arguments (e.g. to a constructor during emplacement).

Sample CL: https://codereview.chromium.org/1437943002 (replaces WTF::forward with std::forward)

[Nico Weber]

I think this is fine eventually, but as it's a pretty corner-case thing, maybe not in the first week :-)

I think this is a super confusing function, but I also think we'll only use it in few places like containers and so on, so most people hopefully won't see it. (Using it in normal client code like https://codereview.chromium.org/1437943002/diff/20001/third_party/WebKit/Source/platform/graphics/paint/PaintController.h seems a bit questionable to me.)

On Wed, Nov 11, 2015 at 1:43 PM, Jeremy Roman <jbr...@chromium.org> wrote:

I'd like to permit using std::forward from <utility> for perfect forwarding of arguments (e.g. to a constructor during emplacement).

Sample CL: https://codereview.chromium.org/1437943002 (replaces WTF::forward with std::forward)

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13cgRnfC_h%2BSLy4TrZLu9ZQ%2Brd2qYgjtCo26qfFBEarsmQ%40mail.gmail.com.

[Jeffrey Yasskin]

Would your advice for PaintController be to remove createAndAppend()
entirely and have an append(MovableType), which forwards to a new

ContiguousContainer::allocateAndMove(DerivedElementType elem) {
…;
return *new (allocate(allocSize)) DerivedElementType(std::move(elem));
}

?

> https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiEdTREW1MU5xM8rKosb4rHN%3Dz4Xv8DDruTZrijj-UqSMg%40mail.gmail.com.

[Sam]

Why delay allowing this? It doesn't add anything beyond what WTF::forward does.

I want to do some perfect forwarding, and I would prefer not to add another copy-paste of std::forward.

[Nico Weber]

On Thu, Nov 12, 2015 at 3:57 PM, Sam <sa...@chromium.org> wrote:

Why delay allowing this? It doesn't add anything beyond what WTF::forward does.

I want to do some perfect forwarding

What for?

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/79818424-432c-4f30-b868-a5a705dc1931%40chromium.org.

[Jeremy Roman]

On Thu, Nov 12, 2015 at 10:39 AM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

Would your advice for PaintController be to remove createAndAppend()
entirely and have an append(MovableType), which forwards to a new

ContiguousContainer::allocateAndMove(DerivedElementType elem) {
  …;
  return *new (allocate(allocSize)) DerivedElementType(std::move(elem));
}

?

Well, in that case the move constructor would already be accessible with the existing allocateAndConstruct (because forwarding the rvalue reference works).

I'm not super strongly opposed to changing this case if it really comes down to that, but PaintController is really just wrapping a container here, and it doesn't seem that much of a stretch to allow emplacement here.

 

On Thu, Nov 12, 2015 at 10:17 AM, Nico Weber <tha...@chromium.org> wrote:
> I think this is fine eventually, but as it's a pretty corner-case thing,
> maybe not in the first week :-)
>
> I think this is a super confusing function, but I also think we'll only use
> it in few places like containers and so on, so most people hopefully won't
> see it. (Using it in normal client code like
> https://codereview.chromium.org/1437943002/diff/20001/third_party/WebKit/Source/platform/graphics/paint/PaintController.h
> seems a bit questionable to me.)

As I said above, I don't think this case is that bad. I'm a little less sold on the ChromeClient use (IMHO it should take a functor/lambda rather than taking a pointer, method pointer and forwarded args).

But I think the Optional case is more compelling, because the scoped objects it is used for aren't movable (and I don't think they should be).

[Sam McNally]

https://codereview.chromium.org/1411063007/diff/90001/mojo/common/service_set.h

[Dana Jansens]

On Thu, Nov 12, 2015 at 4:43 PM, Jeremy Roman <jbr...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 10:39 AM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

Would your advice for PaintController be to remove createAndAppend()
entirely and have an append(MovableType), which forwards to a new

ContiguousContainer::allocateAndMove(DerivedElementType elem) {
  …;
  return *new (allocate(allocSize)) DerivedElementType(std::move(elem));
}

?

Well, in that case the move constructor would already be accessible with the existing allocateAndConstruct (because forwarding the rvalue reference works).

I'm not super strongly opposed to changing this case if it really comes down to that, but PaintController is really just wrapping a container here, and it doesn't seem that much of a stretch to allow emplacement here.

I think it makes sense to use forward here instead of passing a constructed item to the allocate() method. emplace is (going to be) a standard/common idiom in our code base, exposing it on classes that wrap/are a container makes sense to me.

I guess that I am less worried about people using forward() than I am about people writing templated methods that use T&&. If they use forward() as a replacement for move() in those cases then the right things happen, so I mostly want people to make that connection in their heads.

In this particular case, I have additional domain knowledge that there are 1000s to 10s-of-1000s of these items constructed each frame. Forwarding along constructor arguments seems to me like a pretty reasonable thing to do, and preferable to making an object and shallow-copying it into a container for each append.

An alternate implementation that we have in src/cc/ default constructs the members in the container, then calls Set(..constructor args..) as a second method call. Just doing it all in the constructor the way this is done in this case in blink seemed superior to me at the time it was written.

 

 

On Thu, Nov 12, 2015 at 10:17 AM, Nico Weber <tha...@chromium.org> wrote:
> I think this is fine eventually, but as it's a pretty corner-case thing,
> maybe not in the first week :-)
>
> I think this is a super confusing function, but I also think we'll only use
> it in few places like containers and so on, so most people hopefully won't
> see it. (Using it in normal client code like
> https://codereview.chromium.org/1437943002/diff/20001/third_party/WebKit/Source/platform/graphics/paint/PaintController.h
> seems a bit questionable to me.)

As I said above, I don't think this case is that bad. I'm a little less sold on the ChromeClient use (IMHO it should take a functor/lambda rather than taking a pointer, method pointer and forwarded args).

But I think the Optional case is more compelling, because the scoped objects it is used for aren't movable (and I don't think they should be).

Optional seems a bit orthogonal, as I believe that should just match the proposed standard as closely as is possible.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13d8_K6LbhLO3pEaWFbk7NPL7%3D9U%2BVTHOBj_cRu3_O0K3w%40mail.gmail.com.

[Vladimir Levin]

On Fri, Nov 13, 2015 at 1:48 PM, 'Dana Jansens' via cxx <c...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 4:43 PM, Jeremy Roman <jbr...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 10:39 AM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

Would your advice for PaintController be to remove createAndAppend()
entirely and have an append(MovableType), which forwards to a new

ContiguousContainer::allocateAndMove(DerivedElementType elem) {
  …;
  return *new (allocate(allocSize)) DerivedElementType(std::move(elem));
}

?

Well, in that case the move constructor would already be accessible with the existing allocateAndConstruct (because forwarding the rvalue reference works).

I'm not super strongly opposed to changing this case if it really comes down to that, but PaintController is really just wrapping a container here, and it doesn't seem that much of a stretch to allow emplacement here.

I think it makes sense to use forward here instead of passing a constructed item to the allocate() method. emplace is (going to be) a standard/common idiom in our code base, exposing it on classes that wrap/are a container makes sense to me.

I guess that I am less worried about people using forward() than I am about people writing templated methods that use T&&. If they use forward() as a replacement for move() in those cases then the right things happen, so I mostly want people to make that connection in their heads.

In this particular case, I have additional domain knowledge that there are 1000s to 10s-of-1000s of these items constructed each frame. Forwarding along constructor arguments seems to me like a pretty reasonable thing to do, and preferable to making an object and shallow-copying it into a container for each append. 

An alternate implementation that we have in src/cc/ default constructs the members in the container, then calls Set(..constructor args..) as a second method call. Just doing it all in the constructor the way this is done in this case in blink seemed superior to me at the time it was written.

Not that I'm suggesting we do this, but I think having a createAndAppend(const Args&...) would also work, unless move only types are involved here. I think Args&&... with a forward is certainly better, but const Args&... would give the caller the same syntax (right?). That being said, I think we should definitely allow forward for these types of situations (emplace). 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQVUBbRzAYdScqrsc%2Bk9nbUxwLWBd6Yr7QSRrL15owK_A%40mail.gmail.com.

[Mikhail Pozdnyakov]

On Friday, November 13, 2015 at 2:02:22 AM UTC+2, Nico Weber wrote:

On Thu, Nov 12, 2015 at 3:57 PM, Sam <sa...@chromium.org> wrote:

Why delay allowing this? It doesn't add anything beyond what WTF::forward does.

I want to do some perfect forwarding

What for?

WTF containers could be good candidates for it, if they switch to accepting the arguments as universal references.
 

[Mikhail Pozdnyakov]

An example of using std::forward in WTF can be found at https://codereview.chromium.org/1505773002/. PTAL.

[Jeremy Roman]

Update: std::forward is now allowed by https://chromium-cpp.appspot.com/, though usage should be rare (primarily for constructor arguments or very general library code).