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 2018/08/03 21:27:48 UTC
Review Request 68197: Added move support for `class Resources`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68197/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Repository: mesos
Description
-------
See summary.
Diffs
-----
include/mesos/resources.hpp 175833c7d08443955d0aacd844c7df180d43e829
src/common/resources.cpp 253b8bcd720e38f485b5cd2f5b7666ac85e67d38
Diff: https://reviews.apache.org/r/68197/diff/1/
Testing
-------
Thanks,
Meng Zhu
Re: Review Request 68197: Added move support for `class Resources`.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68197/#review206855
-----------------------------------------------------------
PASS: Mesos patch 68197 was successfully built and tested.
Reviews applied: `['68196', '68197']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2041/mesos-review-68197
- Mesos Reviewbot Windows
On Aug. 3, 2018, 9:27 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68197/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2018, 9:27 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 175833c7d08443955d0aacd844c7df180d43e829
> src/common/resources.cpp 253b8bcd720e38f485b5cd2f5b7666ac85e67d38
>
>
> Diff: https://reviews.apache.org/r/68197/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 68197: Added move support for `class Resources`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68197/#review206925
-----------------------------------------------------------
src/common/resources.cpp
Lines 2046-2051 (patched)
<https://reviews.apache.org/r/68197/#comment290072>
Just realized that the following would be more efficient:
```
Resources Resources::operator+(Resources&& that) const
{
Resources result = std::move(that);
result += *this;
return result;
}
```
That way we move then add instead of copy then add; if the resource names are the same (which is usually the case) then this is helpful.
- Benjamin Mahler
On Aug. 6, 2018, 11:15 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68197/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 11:15 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9110
> https://issues.apache.org/jira/browse/MESOS-9110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 21aaf0d512bb74aa08398c9aa432f53fffdd3ff0
> include/mesos/v1/resources.hpp 2f9c704e92d00f55231272fd1ff5654ee8f69eec
> src/common/resources.cpp 253b8bcd720e38f485b5cd2f5b7666ac85e67d38
> src/v1/resources.cpp ab8fc3e738038b9b34d4902aed9f15a59b416217
>
>
> Diff: https://reviews.apache.org/r/68197/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 68197: Added move support for `class Resources`.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68197/
-----------------------------------------------------------
(Updated Aug. 6, 2018, 4:15 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Added a TODO and adjust the declaration order per the review comments.
Bugs: MESOS-9110
https://issues.apache.org/jira/browse/MESOS-9110
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/resources.hpp 21aaf0d512bb74aa08398c9aa432f53fffdd3ff0
include/mesos/v1/resources.hpp 2f9c704e92d00f55231272fd1ff5654ee8f69eec
src/common/resources.cpp 253b8bcd720e38f485b5cd2f5b7666ac85e67d38
src/v1/resources.cpp ab8fc3e738038b9b34d4902aed9f15a59b416217
Diff: https://reviews.apache.org/r/68197/diff/3/
Changes: https://reviews.apache.org/r/68197/diff/2-3/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 68197: Added move support for `class Resources`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68197/#review206910
-----------------------------------------------------------
Fix it, then Ship it!
Looks good! Just a note below about the extra move case for the + operator that helps us avoid a copy, you can either incorporate it into this patch or leave a TODO here and we can land the additional + operators in a follow up.
include/mesos/v1/resources.hpp
Lines 603-607 (original), 615-623 (patched)
<https://reviews.apache.org/r/68197/#comment290040>
It probably makes more sense to group by type? that's also how I would want to read the definitions, side-by-side for the same type:
```
Resources operator+(const Resource& that) const;
Resources operator+(Resource&& that) const;
Resources& operator+=(const Resource& that);
Resources& operator+=(Resource&& that);
Resources operator+(const Resources& that) const;
Resources operator+(Resources&& that) const;
Resources& operator+=(const Resources& that);
Resources& operator+=(Resources&& that);
```
Looks like you already do this grouping in the definitions
include/mesos/v1/resources.hpp
Lines 620-621 (patched)
<https://reviews.apache.org/r/68197/#comment290038>
There are additional benefits to overloading for when the `lhs` is also an rvalue reference, e.g. non-member functions:
https://en.cppreference.com/w/cpp/string/basic_string/operator%2B
When a member function, it looks like this:
https://github.com/apache/mesos/blob/1.6.1/3rdparty/stout/include/stout/option.hpp#L118-L121
We don't need to do it in this patch, but we should at least add the TODO here if we defer it.
- Benjamin Mahler
On Aug. 6, 2018, 4:48 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68197/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 4:48 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9110
> https://issues.apache.org/jira/browse/MESOS-9110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 21aaf0d512bb74aa08398c9aa432f53fffdd3ff0
> include/mesos/v1/resources.hpp 2f9c704e92d00f55231272fd1ff5654ee8f69eec
> src/common/resources.cpp 253b8bcd720e38f485b5cd2f5b7666ac85e67d38
> src/v1/resources.cpp ab8fc3e738038b9b34d4902aed9f15a59b416217
>
>
> Diff: https://reviews.apache.org/r/68197/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 68197: Added move support for `class Resources`.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68197/
-----------------------------------------------------------
(Updated Aug. 6, 2018, 9:48 a.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-9110
https://issues.apache.org/jira/browse/MESOS-9110
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/resources.hpp 21aaf0d512bb74aa08398c9aa432f53fffdd3ff0
include/mesos/v1/resources.hpp 2f9c704e92d00f55231272fd1ff5654ee8f69eec
src/common/resources.cpp 253b8bcd720e38f485b5cd2f5b7666ac85e67d38
src/v1/resources.cpp ab8fc3e738038b9b34d4902aed9f15a59b416217
Diff: https://reviews.apache.org/r/68197/diff/2/
Changes: https://reviews.apache.org/r/68197/diff/1-2/
Testing
-------
make check
Thanks,
Meng Zhu