You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2019/03/04 22:26:25 UTC
Review Request 70123: Added rvalue overload for `Result::get()`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70123/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Benjamin Mahler.
Repository: mesos
Description
-------
Added rvalue overload for `Result::get()`.
Diffs
-----
3rdparty/stout/include/stout/result.hpp c1387ae957edffc31250b9b236b5f1fd8ff0acd3
Diff: https://reviews.apache.org/r/70123/diff/1/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 70123: Added rvalue overload for `Result::get()`.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70123/
-----------------------------------------------------------
(Updated March 8, 2019, 5:50 p.m.)
Review request for mesos, Benjamin Bannier and Benjamin Mahler.
Changes
-------
Addressed Bbannier's comment.
Repository: mesos
Description
-------
Added rvalue overload for `Result::get()`.
Diffs (updated)
-----
3rdparty/stout/include/stout/result.hpp c1387ae957edffc31250b9b236b5f1fd8ff0acd3
Diff: https://reviews.apache.org/r/70123/diff/3/
Changes: https://reviews.apache.org/r/70123/diff/2-3/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 70123: Added rvalue overload for `Result::get()`.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On March 5, 2019, 11:09 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/result.hpp
> > Lines 129 (patched)
> > <https://reviews.apache.org/r/70123/diff/2/?file=2128782#file2128782line129>
> >
> > ```
> > -> decltype(std::forward<Self>(self)
> > .data
> > .get() // NOLINT(mesos-redundant-get)
> > .get())
> > ```
> >
> > See below.
>
> Benjamin Mahler wrote:
> Can't we just use two star operators here given the other outstanding patches?
Ultimately yes, but doing it with just these two patches would introduce a cyclic dependency. We can clean this up later should we remove `get` for good.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70123/#review213433
-----------------------------------------------------------
On March 5, 2019, 12:53 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70123/
> -----------------------------------------------------------
>
> (Updated March 5, 2019, 12:53 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added rvalue overload for `Result::get()`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/result.hpp c1387ae957edffc31250b9b236b5f1fd8ff0acd3
>
>
> Diff: https://reviews.apache.org/r/70123/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70123: Added rvalue overload for `Result::get()`.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On March 5, 2019, 11:09 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/result.hpp
> > Line 126 (original), 140 (patched)
> > <https://reviews.apache.org/r/70123/diff/2/?file=2128782#file2128782line140>
> >
> > The `mesos-redundant-get` clang-tidy check will incorrectly suggest to use `std::forward<Self>(self).data->get()` here which I am not sure how to fix (this function will work on both rvalue and lvalue `self` values which AFAICT don't propagate the same way through `operator->`.
> >
> > I'd suggest we add a linter suppression here for now until we possibly get rid of `get` in favor of `operator*` for good.
> > ```
> > return std::forward<Self>(self)
> > .data
> > .get() // NOLINT(mesos-redundant-get)
> > .get();
> > ```
>
> Benjamin Bannier wrote:
> Actually there's also `NOLINTNEXTLINT` which would allow us to keep the expression on a single line, e.g.,
> ```
> // NOTLINENEXTLINE(mesos-redundant-get)
> return std::forward<Self>(self).data.get().get()
> ```
>
> Also above.
Should have been `NOLINTNEXTLINE`.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70123/#review213433
-----------------------------------------------------------
On March 5, 2019, 12:53 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70123/
> -----------------------------------------------------------
>
> (Updated March 5, 2019, 12:53 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added rvalue overload for `Result::get()`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/result.hpp c1387ae957edffc31250b9b236b5f1fd8ff0acd3
>
>
> Diff: https://reviews.apache.org/r/70123/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70123: Added rvalue overload for `Result::get()`.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On March 5, 2019, 11:09 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/result.hpp
> > Line 126 (original), 140 (patched)
> > <https://reviews.apache.org/r/70123/diff/2/?file=2128782#file2128782line140>
> >
> > The `mesos-redundant-get` clang-tidy check will incorrectly suggest to use `std::forward<Self>(self).data->get()` here which I am not sure how to fix (this function will work on both rvalue and lvalue `self` values which AFAICT don't propagate the same way through `operator->`.
> >
> > I'd suggest we add a linter suppression here for now until we possibly get rid of `get` in favor of `operator*` for good.
> > ```
> > return std::forward<Self>(self)
> > .data
> > .get() // NOLINT(mesos-redundant-get)
> > .get();
> > ```
Actually there's also `NOLINTNEXTLINT` which would allow us to keep the expression on a single line, e.g.,
```
// NOTLINENEXTLINE(mesos-redundant-get)
return std::forward<Self>(self).data.get().get()
```
Also above.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70123/#review213433
-----------------------------------------------------------
On March 5, 2019, 12:53 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70123/
> -----------------------------------------------------------
>
> (Updated March 5, 2019, 12:53 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added rvalue overload for `Result::get()`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/result.hpp c1387ae957edffc31250b9b236b5f1fd8ff0acd3
>
>
> Diff: https://reviews.apache.org/r/70123/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70123: Added rvalue overload for `Result::get()`.
Posted by Benjamin Mahler <bm...@apache.org>.
> On March 5, 2019, 10:09 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/result.hpp
> > Lines 129 (patched)
> > <https://reviews.apache.org/r/70123/diff/2/?file=2128782#file2128782line129>
> >
> > ```
> > -> decltype(std::forward<Self>(self)
> > .data
> > .get() // NOLINT(mesos-redundant-get)
> > .get())
> > ```
> >
> > See below.
Can't we just use two star operators here given the other outstanding patches?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70123/#review213433
-----------------------------------------------------------
On March 4, 2019, 11:53 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70123/
> -----------------------------------------------------------
>
> (Updated March 4, 2019, 11:53 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added rvalue overload for `Result::get()`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/result.hpp c1387ae957edffc31250b9b236b5f1fd8ff0acd3
>
>
> Diff: https://reviews.apache.org/r/70123/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70123: Added rvalue overload for `Result::get()`.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70123/#review213433
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/stout/include/stout/result.hpp
Lines 128 (patched)
<https://reviews.apache.org/r/70123/#comment299387>
Maybe worth adding a comment that this is not a member function so we can decouple us from the `const`-ness of `this` (if I follow the reasoning correctly).
3rdparty/stout/include/stout/result.hpp
Lines 129 (patched)
<https://reviews.apache.org/r/70123/#comment299385>
```
-> decltype(std::forward<Self>(self)
.data
.get() // NOLINT(mesos-redundant-get)
.get())
```
See below.
3rdparty/stout/include/stout/result.hpp
Line 126 (original), 140 (patched)
<https://reviews.apache.org/r/70123/#comment299384>
The `mesos-redundant-get` clang-tidy check will incorrectly suggest to use `std::forward<Self>(self).data->get()` here which I am not sure how to fix (this function will work on both rvalue and lvalue `self` values which AFAICT don't propagate the same way through `operator->`.
I'd suggest we add a linter suppression here for now until we possibly get rid of `get` in favor of `operator*` for good.
```
return std::forward<Self>(self)
.data
.get() // NOLINT(mesos-redundant-get)
.get();
```
3rdparty/stout/include/stout/result.hpp
Line 131 (original)
<https://reviews.apache.org/r/70123/#comment299386>
Great we were able to get rid of this!
- Benjamin Bannier
On March 5, 2019, 12:53 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70123/
> -----------------------------------------------------------
>
> (Updated March 5, 2019, 12:53 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added rvalue overload for `Result::get()`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/result.hpp c1387ae957edffc31250b9b236b5f1fd8ff0acd3
>
>
> Diff: https://reviews.apache.org/r/70123/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70123: Added rvalue overload for `Result::get()`.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70123/
-----------------------------------------------------------
(Updated March 4, 2019, 3:53 p.m.)
Review request for mesos, Benjamin Bannier and Benjamin Mahler.
Repository: mesos
Description
-------
Added rvalue overload for `Result::get()`.
Diffs (updated)
-----
3rdparty/stout/include/stout/result.hpp c1387ae957edffc31250b9b236b5f1fd8ff0acd3
Diff: https://reviews.apache.org/r/70123/diff/2/
Changes: https://reviews.apache.org/r/70123/diff/1-2/
Testing
-------
make check
Thanks,
Meng Zhu