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


Looking much better, thanks Kapil!!


3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/25597/#comment93251>

    What about s/versionStrings/split/ as the variable name?



3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/25597/#comment93244>

    Any tests for the error cases?
    
    For example, a test for "0.1.2.3" would cause this code to crash, because 
it will go over the end of the array.



3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/25597/#comment93246>

    Can you include the parse error here?
    
    e.g.
    ```
      return Error("Invalid version component '" + versionStrings[i] + ': " + 
numify.error());
    ```
    
    When we return an error string, we typically do not include the input 
because it often leads to double logging, consider what a caller might log:
    
    ```
    Try<Version> version = Version::parse(v);
    
    if (version.isError()) {
      LOG(ERROR) << "Failed to parse version '" << v << "': " 
                 << version.error();
    }
    ```
    
    In this case, the best thing we can do is to return an Error of the 
_reason_ the version was unparsable.



3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/25597/#comment93247>

    Another newline here :)



3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/25597/#comment93248>

    Missing <ostream> include?



3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp
<https://reviews.apache.org/r/25597/#comment93249>

    Some error cases would be great here!


- Ben Mahler


On Sept. 16, 2014, 5:11 a.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25597/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2014, 5:11 a.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Currently there is no facility in Mesos for checking compatibility of various 
> Mesos components that could have been built at different times with 
> potentially different Mesos versions.  This requirement is especially 
> important for doing various compatibility checks between Mesos and Mesos 
> modules (WIP).
> 
> - Features major, minor, and patch numbers.
> - Convenience functions for comparing two versions.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> db9766d70adb9076946cd2b467c55636fe5f7235 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> b6464de53c3873ecd0b62a08ca9aac530043ffb9 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 6fa5b741bdd7f089ba93bf6fea43b9f39f8f0edb 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 5bbf829b3fa5d09a92e1d64c52c1fc7eed73fc91 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/25597/diff/
> 
> 
> Testing
> -------
> 
> Added a stout test and ran make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Reply via email to