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