You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mp...@apache.org> on 2015/12/16 20:38:57 UTC
Review Request 41460: Used `is_bind_expression` to SFINAE correctly.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41460/
-----------------------------------------------------------
Review request for mesos, Alex Clemmer and Joris Van Remoortere.
Repository: mesos
Description
-------
The Standard (C++11 through 17) does not require `std::bind`'s function call operator to SFINAE, and VS 2015's doesn't. `is_bind_expression` can be used to manually reroute bind expressions to the 1-arg overload, where (conveniently) the argument will be ignored if necessary.
Follow-up from [r40114](https://reviews.apache.org/r/40114/).
Diffs
-----
3rdparty/libprocess/include/process/future.hpp c9146e3a3ccf09dd37c5a8ac7000fbe84f3c710c
Diff: https://reviews.apache.org/r/41460/diff/
Testing
-------
`make check` on OS X, compiled on Windows.
Thanks,
Michael Park
Re: Review Request 41460: Used `std::is_bind_expression` to SFINAE
correctly.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 3, 2016, 11:55 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/future.hpp, lines 228-230
> > <https://reviews.apache.org/r/41460/diff/1/?file=1166979#file1166979line228>
> >
> > Why do we need to capture/alias the type `F` as `G` again and then use it in `std::result_of<G()>::type`? Why can't we just use `F` there again?
>
> Michael Park wrote:
> We could if there was a guarantee that the default template arguments are instantiated left-to-right. I'm not sure whether such guarantee exists and would prefer not rely on it even if it did.
>
> This way, it's required that the default argument to `G` be instantiated first, and there's no chance of `std::result_of` being instantiated with `F` if `F` is the result of a `std::bind`.
Synced with BenH offline about this. The SFINAE evaluation is not guaranteed to be performed in lexical order until C++14. Once we get to C++14 we can write something like:
```cpp
template <
typename F,
typename = typename std::enable_if<!std::is_bind_expression<
typename std::decay<F>::type>::value>::type,
typename = typename std::result_of<F()>::type>
...
```
Meanwhile, we simply inline the `G`.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41460/#review112480
-----------------------------------------------------------
On Jan. 6, 2016, 2:27 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41460/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2016, 2:27 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Alex Clemmer, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4228
> https://issues.apache.org/jira/browse/MESOS-4228
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Standard (C++11 through 17) does not require `std::bind`'s function call operator to SFINAE, and VS 2015's doesn't. `std::is_bind_expression` can be used to manually reroute bind expressions to the 1-arg overload, where (conveniently) the argument will be ignored if necessary.
>
> Follow-up from [r40114](https://reviews.apache.org/r/40114/).
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp bcb5668565298825056f1b48d48efe12d2e56e7c
>
> Diff: https://reviews.apache.org/r/41460/diff/
>
>
> Testing
> -------
>
> `make check` on OS X, compiled on Windows.
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 41460: Used `std::is_bind_expression` to SFINAE
correctly.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 3, 2016, 11:55 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/future.hpp, line 226
> > <https://reviews.apache.org/r/41460/diff/1/?file=1166979#file1166979line226>
> >
> > I'd love to see a comment here that says:
> >
> > This is the less prefered `onReady`, we prefer the `onReady` method which has `f` taking a `const T&` parameter. Unfortunately, to complicate matters, if `F` is a `std::bind` expression we need to SFINAE out this version of `onReady` and force the use of the preferred `onReady` (which thankfully works because `std::bind` will just ignore the `const T&` argument). This is necessary because Visual Studio 2015 doesn't support using the `std::bind` call operator with `std::result_of` as it's technically not a requirement by the C++ standard.
> >
> > And then let's add a comment over the other `LessPrefer` variants that points up to the `onReady(F f, LessPrefer)` that includes this comment.
Added.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41460/#review112480
-----------------------------------------------------------
On Jan. 5, 2016, 11:58 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41460/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2016, 11:58 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Alex Clemmer, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4228
> https://issues.apache.org/jira/browse/MESOS-4228
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Standard (C++11 through 17) does not require `std::bind`'s function call operator to SFINAE, and VS 2015's doesn't. `std::is_bind_expression` can be used to manually reroute bind expressions to the 1-arg overload, where (conveniently) the argument will be ignored if necessary.
>
> Follow-up from [r40114](https://reviews.apache.org/r/40114/).
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp bcb5668565298825056f1b48d48efe12d2e56e7c
>
> Diff: https://reviews.apache.org/r/41460/diff/
>
>
> Testing
> -------
>
> `make check` on OS X, compiled on Windows.
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 41460: Used `std::is_bind_expression` to SFINAE
correctly.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 3, 2016, 11:55 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/future.hpp, lines 220-223
> > <https://reviews.apache.org/r/41460/diff/1/?file=1166979#file1166979line220>
> >
> > Now that we're not taking `f` as a universal reference for each of these methods it seems that we might make two copies (one when we invoke the function and the second when we capture `f` in the lambda)?
> >
> > Can we add a TODO to move capture `f` once we have C++14? Or alternatively should we use `std::bind` here so we can explicitly move it and then convert back to lambda syntax once we have C++14?
Yeah, there are a few places where we potentially copy twice. I guess my understanding was that we typically don't worry about performance unless it becomes a clear problem. I think maybe what we do in this review is keep the forwarding references for now and talk about the "pass-by-value & move" pattern separately.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41460/#review112480
-----------------------------------------------------------
On Jan. 5, 2016, 11:58 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41460/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2016, 11:58 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Alex Clemmer, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4228
> https://issues.apache.org/jira/browse/MESOS-4228
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Standard (C++11 through 17) does not require `std::bind`'s function call operator to SFINAE, and VS 2015's doesn't. `std::is_bind_expression` can be used to manually reroute bind expressions to the 1-arg overload, where (conveniently) the argument will be ignored if necessary.
>
> Follow-up from [r40114](https://reviews.apache.org/r/40114/).
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp bcb5668565298825056f1b48d48efe12d2e56e7c
>
> Diff: https://reviews.apache.org/r/41460/diff/
>
>
> Testing
> -------
>
> `make check` on OS X, compiled on Windows.
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 41460: Used `std::is_bind_expression` to SFINAE
correctly.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 3, 2016, 11:55 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/future.hpp, lines 228-230
> > <https://reviews.apache.org/r/41460/diff/1/?file=1166979#file1166979line228>
> >
> > Why do we need to capture/alias the type `F` as `G` again and then use it in `std::result_of<G()>::type`? Why can't we just use `F` there again?
We could if there was a guarantee that the default template arguments are instantiated left-to-right. I'm not sure whether such guarantee exists and would prefer not rely on it even if it did.
This way, it's required that the default argument to `G` be instantiated first, and there's no chance of `std::result_of` being instantiated with `F` if `F` is the result of a `std::bind`.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41460/#review112480
-----------------------------------------------------------
On Jan. 5, 2016, 11:58 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41460/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2016, 11:58 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Alex Clemmer, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4228
> https://issues.apache.org/jira/browse/MESOS-4228
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Standard (C++11 through 17) does not require `std::bind`'s function call operator to SFINAE, and VS 2015's doesn't. `std::is_bind_expression` can be used to manually reroute bind expressions to the 1-arg overload, where (conveniently) the argument will be ignored if necessary.
>
> Follow-up from [r40114](https://reviews.apache.org/r/40114/).
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp bcb5668565298825056f1b48d48efe12d2e56e7c
>
> Diff: https://reviews.apache.org/r/41460/diff/
>
>
> Testing
> -------
>
> `make check` on OS X, compiled on Windows.
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 41460: Used `std::is_bind_expression` to SFINAE
correctly.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 3, 2016, 11:55 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/future.hpp, line 291
> > <https://reviews.apache.org/r/41460/diff/1/?file=1166979#file1166979line291>
> >
> > This is another spot that looks like we could have a double copy?
See above: https://reviews.apache.org/r/41460/#comment172929
> On Jan. 3, 2016, 11:55 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/future.hpp, line 300
> > <https://reviews.apache.org/r/41460/diff/1/?file=1166979#file1166979line300>
> >
> > Do you know which compilers will elide this `std::move`?
See above: https://reviews.apache.org/r/41460/#comment172929
> On Jan. 3, 2016, 11:55 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/future.hpp, lines 220-223
> > <https://reviews.apache.org/r/41460/diff/1/?file=1166979#file1166979line220>
> >
> > Now that we're not taking `f` as a universal reference for each of these methods it seems that we might make two copies (one when we invoke the function and the second when we capture `f` in the lambda)?
> >
> > Can we add a TODO to move capture `f` once we have C++14? Or alternatively should we use `std::bind` here so we can explicitly move it and then convert back to lambda syntax once we have C++14?
>
> Michael Park wrote:
> Yeah, there are a few places where we potentially copy twice. I guess my understanding was that we typically don't worry about performance unless it becomes a clear problem. I think maybe what we do in this review is keep the forwarding references for now and talk about the "pass-by-value & move" pattern separately.
Reverted to using forwarding references as before. We'll have a discussion around whether we want to adopt the "pass-by-value + move" pattern project-wide.
> On Jan. 3, 2016, 11:55 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/future.hpp, line 312
> > <https://reviews.apache.org/r/41460/diff/1/?file=1166979#file1166979line312>
> >
> > Double copy?
See above: https://reviews.apache.org/r/41460/#comment172929
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41460/#review112480
-----------------------------------------------------------
On Jan. 5, 2016, 11:58 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41460/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2016, 11:58 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Alex Clemmer, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4228
> https://issues.apache.org/jira/browse/MESOS-4228
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Standard (C++11 through 17) does not require `std::bind`'s function call operator to SFINAE, and VS 2015's doesn't. `std::is_bind_expression` can be used to manually reroute bind expressions to the 1-arg overload, where (conveniently) the argument will be ignored if necessary.
>
> Follow-up from [r40114](https://reviews.apache.org/r/40114/).
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp bcb5668565298825056f1b48d48efe12d2e56e7c
>
> Diff: https://reviews.apache.org/r/41460/diff/
>
>
> Testing
> -------
>
> `make check` on OS X, compiled on Windows.
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 41460: Used `std::is_bind_expression` to SFINAE
correctly.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41460/#review112480
-----------------------------------------------------------
3rdparty/libprocess/include/process/future.hpp (lines 220 - 223)
<https://reviews.apache.org/r/41460/#comment172929>
Now that we're not taking `f` as a universal reference for each of these methods it seems that we might make two copies (one when we invoke the function and the second when we capture `f` in the lambda)?
Can we add a TODO to move capture `f` once we have C++14? Or alternatively should we use `std::bind` here so we can explicitly move it and then convert back to lambda syntax once we have C++14?
3rdparty/libprocess/include/process/future.hpp (line 226)
<https://reviews.apache.org/r/41460/#comment172931>
I'd love to see a comment here that says:
This is the less prefered `onReady`, we prefer the `onReady` method which has `f` taking a `const T&` parameter. Unfortunately, to complicate matters, if `F` is a `std::bind` expression we need to SFINAE out this version of `onReady` and force the use of the preferred `onReady` (which thankfully works because `std::bind` will just ignore the `const T&` argument). This is necessary because Visual Studio 2015 doesn't support using the `std::bind` call operator with `std::result_of` as it's technically not a requirement by the C++ standard.
And then let's add a comment over the other `LessPrefer` variants that points up to the `onReady(F f, LessPrefer)` that includes this comment.
3rdparty/libprocess/include/process/future.hpp (lines 228 - 230)
<https://reviews.apache.org/r/41460/#comment172930>
Why do we need to capture/alias the type `F` as `G` again and then use it in `std::result_of<G()>::type`? Why can't we just use `F` there again?
3rdparty/libprocess/include/process/future.hpp (line 291)
<https://reviews.apache.org/r/41460/#comment172933>
This is another spot that looks like we could have a double copy?
3rdparty/libprocess/include/process/future.hpp (line 297)
<https://reviews.apache.org/r/41460/#comment172932>
Do you know which compilers will elide this `std::move`?
3rdparty/libprocess/include/process/future.hpp (line 309)
<https://reviews.apache.org/r/41460/#comment172934>
Double copy?
- Benjamin Hindman
On Dec. 28, 2015, 3:42 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41460/
> -----------------------------------------------------------
>
> (Updated Dec. 28, 2015, 3:42 p.m.)
>
>
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
>
>
> Bugs: MESOS-4228
> https://issues.apache.org/jira/browse/MESOS-4228
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Standard (C++11 through 17) does not require `std::bind`'s function call operator to SFINAE, and VS 2015's doesn't. `std::is_bind_expression` can be used to manually reroute bind expressions to the 1-arg overload, where (conveniently) the argument will be ignored if necessary.
>
> Follow-up from [r40114](https://reviews.apache.org/r/40114/).
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp c9146e3a3ccf09dd37c5a8ac7000fbe84f3c710c
>
> Diff: https://reviews.apache.org/r/41460/diff/
>
>
> Testing
> -------
>
> `make check` on OS X, compiled on Windows.
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 41460: Used `std::is_bind_expression` to SFINAE
correctly.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41460/
-----------------------------------------------------------
(Updated Jan. 5, 2016, 11:58 p.m.)
Review request for mesos, Alex Clemmer and Joris Van Remoortere.
Changes
-------
Rebased.
Bugs: MESOS-4228
https://issues.apache.org/jira/browse/MESOS-4228
Repository: mesos
Description
-------
The Standard (C++11 through 17) does not require `std::bind`'s function call operator to SFINAE, and VS 2015's doesn't. `std::is_bind_expression` can be used to manually reroute bind expressions to the 1-arg overload, where (conveniently) the argument will be ignored if necessary.
Follow-up from [r40114](https://reviews.apache.org/r/40114/).
Diffs (updated)
-----
3rdparty/libprocess/include/process/future.hpp bcb5668565298825056f1b48d48efe12d2e56e7c
Diff: https://reviews.apache.org/r/41460/diff/
Testing
-------
`make check` on OS X, compiled on Windows.
Thanks,
Michael Park
Re: Review Request 41460: Used `std::is_bind_expression` to SFINAE
correctly.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41460/
-----------------------------------------------------------
(Updated Dec. 28, 2015, 3:42 p.m.)
Review request for mesos, Alex Clemmer and Joris Van Remoortere.
Bugs: MESOS-4228
https://issues.apache.org/jira/browse/MESOS-4228
Repository: mesos
Description
-------
The Standard (C++11 through 17) does not require `std::bind`'s function call operator to SFINAE, and VS 2015's doesn't. `std::is_bind_expression` can be used to manually reroute bind expressions to the 1-arg overload, where (conveniently) the argument will be ignored if necessary.
Follow-up from [r40114](https://reviews.apache.org/r/40114/).
Diffs
-----
3rdparty/libprocess/include/process/future.hpp c9146e3a3ccf09dd37c5a8ac7000fbe84f3c710c
Diff: https://reviews.apache.org/r/41460/diff/
Testing
-------
`make check` on OS X, compiled on Windows.
Thanks,
Michael Park
Re: Review Request 41460: Used `std::is_bind_expression` to SFINAE
correctly.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41460/
-----------------------------------------------------------
(Updated Dec. 16, 2015, 7:44 p.m.)
Review request for mesos, Alex Clemmer and Joris Van Remoortere.
Summary (updated)
-----------------
Used `std::is_bind_expression` to SFINAE correctly.
Repository: mesos
Description (updated)
-------
The Standard (C++11 through 17) does not require `std::bind`'s function call operator to SFINAE, and VS 2015's doesn't. `std::is_bind_expression` can be used to manually reroute bind expressions to the 1-arg overload, where (conveniently) the argument will be ignored if necessary.
Follow-up from [r40114](https://reviews.apache.org/r/40114/).
Diffs
-----
3rdparty/libprocess/include/process/future.hpp c9146e3a3ccf09dd37c5a8ac7000fbe84f3c710c
Diff: https://reviews.apache.org/r/41460/diff/
Testing
-------
`make check` on OS X, compiled on Windows.
Thanks,
Michael Park