mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Mathiske" <be...@mesosphere.io>
Subject Re: Review Request 34278: Refactor Stout Result<T> leveraging Try<Option<T>> to remove the dynamic allocation.
Date Tue, 26 May 2015 15:24:14 GMT

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



3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp
<https://reviews.apache.org/r/34278/#comment136689>

    Please insert a blank line before the return statement.



3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp
<https://reviews.apache.org/r/34278/#comment136691>

    IMHO multiple statements read better if not on the same line. Even though the original
code has them on the same line. The extra parens introduced with isError() add to this impression
now.



3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp
<https://reviews.apache.org/r/34278/#comment136686>

    Although the second sentence is true and emphasizes an impressive point, being a _relative_
statement, it will be confusing without presenting the inferior alternative, which will be
overwritten by then. Suggestion: delete it.


- Bernd Mathiske


On May 21, 2015, 4:45 p.m., Joris Van Remoortere wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34278/
> -----------------------------------------------------------
> 
> (Updated May 21, 2015, 4:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Cody Maloney, Joerg Schad,
and Michael Park.
> 
> 
> Bugs: MESOS-2740
>     https://issues.apache.org/jira/browse/MESOS-2740
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Aggregate a Try<Option<T>> to leverage the RAII pattern around unrestricted
union.
> Added some comments to Result<T>.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp 96b0f36104bed28cc227904e34cca166435a3d38

> 
> Diff: https://reviews.apache.org/r/34278/diff/
> 
> 
> Testing
> -------
> 
> make check.
> valgrind reports fewer allocations.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>


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