mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <benjamin.mah...@gmail.com>
Subject Re: Review Request 47990: Added move semantics to `Pipe::write()`.
Date Tue, 31 May 2016 17:20:56 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47990/#review135654
-----------------------------------------------------------


Fix it, then Ship it!





3rdparty/libprocess/src/http.cpp (line 459)
<https://reviews.apache.org/r/47990/#comment200670>

    I would remove this variable since it seems to miss the structure of this code:
    
    ```
    if (readers) {
      give to waiting reader
    } else {
      add to writes
    }
    ```
    
    (in the opposite order) It just so happens that we pulled out the future satisfaction
from the first section here because we didn't want to run callbacks from within the critical
section. So this variable is akin to the following:
    
    ```
    bool moved = false;
    if (readers) {
      CHECK(!moved);
      give to waiting reader
    } else {
      moved = true;
      add to writes
    }
    ```
    
    This seems to miss the idea?


- Benjamin Mahler


On May 30, 2016, 11:21 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47990/
> -----------------------------------------------------------
> 
> (Updated May 30, 2016, 11:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Michael Park, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Instead of having separate rvalue overload for `write`,
> this change takes the sink argument by value
> and then does a move. This has the additional small
> overhead of performing an extra move.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/http.hpp 10f6fb90e436300bc47f4d5d2ad4beabd19026ba

>   3rdparty/libprocess/src/http.cpp b7839b1402d0660a2461085f5a1db191296e3308 
> 
> Diff: https://reviews.apache.org/r/47990/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>


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