1355739 - Make MakeScopeExit allow to pass by lvalue callable object

Status: RESOLVED WONTFIX

(Core :: MFBT, enhancement)

Description

[James Cheng[:JamesCheng]]

First I saw these combination

http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/mfbt/ScopeExit.h#128,130

I guess it should be Forward not Move.

But seeing the ScopeExit constructor again, it intends to accept rvalue.

If we pass lvalue into MakeScopeExit(ExitFunction&& exitFunction),

ExitFunction&& will be deduced into ExitFunction& and the 

"explicit ScopeExit(ExitFunction&& cleanup," will be explicit ScopeExit(ExitFunction&&& cleanup which equals to explicit ScopeExit(ExitFunction& cleanup

That will become a lvalue constructor.

I know the trend is to pass rvalue lambda as a callable object but I think passing a lvalue callable should also be acceptable.

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Bug Bug 1355739 - Part1: Pass lvalue callable object into MakeScopeExit should be allowd.

Review commit: https://reviewboard.mozilla.org/r/129352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129352/

Comment 2

[James Cheng[:JamesCheng]]

Attachment: Bug Bug 1355739 - Part2: Add a test case by passing a lvalue callable object.

Review commit: https://reviewboard.mozilla.org/r/129354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129354/

Comment 3

[James Cheng[:JamesCheng]]

Comment on attachment 8857368 [details]
Bug Bug 1355739 - Part2: Add a test case by passing a lvalue callable object.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129354/diff/1-2/

Comment 4

[James Cheng[:JamesCheng]]

Attach treeherder for reference 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea9fe481bb4ba7e0683425fb1659dc687ca48da3

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8857367 [details]
Bug Bug 1355739 - Part1: Pass lvalue callable object into MakeScopeExit should be allowd.

https://reviewboard.mozilla.org/r/129352/#review132208

There do seem to be some problems with how this stuff is currently defined, that are worth fixing independent of everything else.

But as regards the specific concern that motivated filing this: what exactly is your use case for accepting an "lvalue callable object" here?  `MakeScopeExit` really is mostly (possibly entirely) intended for use with lambdas.  It may be that you *could* or *can* use it with other types.  But this isn't currently an intended use case, and that things other than lambdas are accepted in some circumstances is mostly happenstance.  What is the exact real-world case of yours where you want to do what this bug proposes to enable?

It might be that the independent fixing would happen to enable "lvalue callable object" support here.  If so, that's fine.  But if simply applying `Decay` and `Forward` to this don't enable whatever it is you're trying to do, I'm not inclined to enable it -- at least, not unless you can give an example of code you would write, that would use it.  (Make-work in a test is obviously not a real use case.)

So: please provide a use case where you would use this functionality.

::: mfbt/ScopeExit.h:127
(Diff revision 1)
>    ScopeExit& operator=(const ScopeExit&) = delete;
>    ScopeExit& operator=(ScopeExit&&) = delete;
>  };
>  
>  template <typename ExitFunction>
> -ScopeExit<ExitFunction>
> +ScopeExit<typename RemoveConst<typename RemoveReference<ExitFunction>::Type>::Type>

I'm pretty `typename Decay<ExitFunction>::Type` does what you really want to do here.

::: mfbt/ScopeExit.h:133
(Diff revision 1)
>  MakeScopeExit(ExitFunction&& exitFunction)
>  {
> -  return ScopeExit<ExitFunction>(mozilla::Move(exitFunction));
> +  using RemoveCRType =
> +    typename RemoveConst<typename RemoveReference<ExitFunction>::Type>::Type;
> +  RemoveCRType exitFunc = exitFunction;
> +  return ScopeExit<RemoveCRType>(mozilla::Move(exitFunc));

Again here, I think you're mostly duplicating what `Decay` will do for you.  Additionally, you seem to be attempting to perfectly forward the argument -- but you're not using the standard perfect forwarding idiom (see the long comments in mfbt/Move.h for details).  So I think this probably should be

    using Guard = ScopeExit<typename Decay<ExitFunction>::Type>;
    return Guard(mozilla::Forward(exitFunc));

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8857368 [details]
Bug Bug 1355739 - Part2: Add a test case by passing a lvalue callable object.

https://reviewboard.mozilla.org/r/129354/#review132242

This works fine as a test, but please include it directly in the revision that changes ScopeExit.h.  Tests should almost always land in the same revision as the modified code that demanded them.

Comment 7

[James Cheng[:JamesCheng]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Comment on attachment 8857367 [details]
> Bug Bug 1355739 - Part1: Pass lvalue callable object into MakeScopeExit
> should be allowd.
> 
> https://reviewboard.mozilla.org/r/129352/#review132208
> 
> There do seem to be some problems with how this stuff is currently defined,
> that are worth fixing independent of everything else.
> 
> But as regards the specific concern that motivated filing this: what exactly
> is your use case for accepting an "lvalue callable object" here? 
> `MakeScopeExit` really is mostly (possibly entirely) intended for use with
> lambdas.  It may be that you *could* or *can* use it with other types.  But
> this isn't currently an intended use case, and that things other than
> lambdas are accepted in some circumstances is mostly happenstance.  What is
> the exact real-world case of yours where you want to do what this bug
> proposes to enable?
> 
> It might be that the independent fixing would happen to enable "lvalue
> callable object" support here.  If so, that's fine.  But if simply applying
> `Decay` and `Forward` to this don't enable whatever it is you're trying to
> do, I'm not inclined to enable it -- at least, not unless you can give an
> example of code you would write, that would use it.  (Make-work in a test is
> obviously not a real use case.)
> 
> So: please provide a use case where you would use this functionality.
> 
> ::: mfbt/ScopeExit.h:127
> (Diff revision 1)
> >    ScopeExit& operator=(const ScopeExit&) = delete;
> >    ScopeExit& operator=(ScopeExit&&) = delete;
> >  };
> >  
> >  template <typename ExitFunction>
> > -ScopeExit<ExitFunction>
> > +ScopeExit<typename RemoveConst<typename RemoveReference<ExitFunction>::Type>::Type>
> 
> I'm pretty `typename Decay<ExitFunction>::Type` does what you really want to
> do here.
> 
> ::: mfbt/ScopeExit.h:133
> (Diff revision 1)
> >  MakeScopeExit(ExitFunction&& exitFunction)
> >  {
> > -  return ScopeExit<ExitFunction>(mozilla::Move(exitFunction));
> > +  using RemoveCRType =
> > +    typename RemoveConst<typename RemoveReference<ExitFunction>::Type>::Type;
> > +  RemoveCRType exitFunc = exitFunction;
> > +  return ScopeExit<RemoveCRType>(mozilla::Move(exitFunc));
> 
> Again here, I think you're mostly duplicating what `Decay` will do for you. 
> Additionally, you seem to be attempting to perfectly forward the argument --
> but you're not using the standard perfect forwarding idiom (see the long
> comments in mfbt/Move.h for details).  So I think this probably should be
> 
>     using Guard = ScopeExit<typename Decay<ExitFunction>::Type>;
>     return Guard(mozilla::Forward(exitFunc));

Thank for your review, frankly I don't have any use case using lvalue callable object passing to the ScopeExit... I just want to know why it restricts user to pass rvalue.
I'm using Move instead of Forward here because I have already make a copy and it is lvalue, so I used Move.

Comment 8

[Jeff Walden [:Waldo]]

I think you might not fully understand the relevant bits of C++'s template system here.  When you see a function like

  template<typename T>
  void
  Foo(T&& t)
  {
    ...
  }

this function *doesn't* only take rvalue references.  There's a special rule in the template type inference system, applied to |T&&| parameters where |T| is a template parameter *on that same function* (and *not* on an enclosing class), such that you can pass in non-rvalue references.  Then, T gets specially inferred to be just the right type.  (For example, if you pass in an int&, T is inferred to be int&, and then the meaning of T&& is int& && which collapses into int&.)  Moreover, if you want to simply *pass along* |t| to some other function, without using it yourself, so that other function sees exactly what you see, you would pass

  mozilla::Forward<T>(t)

to perfectly forward that argument.

It seems to me, then, that ScopeExit's arguable failings here are that it's not correctly implementing perfect forwarding -- that is, it's using |mozilla::Move(t)| instead of |mozilla::Forward<T>(t)| -- and it's not using |mozilla::Decay|.  But I'm not actually sure, in the absence of a justifying use case, that either of these is truly a problem.

In the absence of a use case, then, I'm going to bias in favor of the status quo and keep this code the way it currently is.  Throwing Decay/Forward at this *might* break some desirable use cases, and it could impose on some use cases.[0]  No reason to take that risk in advance of a clear need for it.

0. Decaying a T& reference will mean that ScopeExit contains a T, for example -- which wouldn't do the right thing if the user assumed modifications to the original T& would be visible in the ExitFunction during that ~ScopeExit.  Some spec things that have similar concerns, require the user to pass in actual T& references within a special reference wrapper class, to specifically identify them as references and exempt them from decay.  But such wrapper is confusing and burdensome.  If I can avoid introducing such a wrapper, I will.

Comment 9

[James Cheng[:JamesCheng]]

Yes, I understand the perfect forwarding rule so I want to replace Move with Forward at the first thought.

As I describe in comment 0
=================================
If we pass lvalue into MakeScopeExit(ExitFunction&& exitFunction),
ExitFunction&& will be deduced into ExitFunction& and the 

"explicit ScopeExit(ExitFunction&& cleanup," will be explicit ScopeExit(ExitFunction&&& cleanup which equals to explicit ScopeExit(ExitFunction& cleanup

That will become a lvalue constructor.
==================================

Since I have copy the exitFunction(ExitFunction& or ExitFunction&& whatever), it becomes lvalue, so I don't need to use Forward in this case.


Thank you.