mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 68197: Added move support for `class Resources`.
Date Mon, 06 Aug 2018 21:19:34 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message