Re: Review Request 68197: Added move support for `class Resources`.

[Benjamin Mahler]

---
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)


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`.

[Meng Zhu]

---
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`.

[Benjamin Mahler]

---
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)


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)


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`.

[Meng Zhu]

---
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

Re: Review Request 68197: Added move support for `class Resources`.

[Mesos Reviewbot Windows]

---
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
> 
>