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