----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25597/#review53370 -----------------------------------------------------------
Thanks for adding this Kapil! I made a few notes for cleanup, in particular, about: (1) The consolidation with os::Release, which implements ~ the same thing. (2) Leveraging stringify with a stream operator as opposed to using a str() operator. (3) I made some style comments and naming suggestions, in particular avoiding abbreviations like 'str' where possible. Looking forward to getting this committed! 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp <https://reviews.apache.org/r/25597/#comment93001> This is roughly the same as os::Release in os.hpp, are you planning to update os::release() to now return a Version? At the very least, please add a TODO on os::release() to leverage Version introduced here. 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp <https://reviews.apache.org/r/25597/#comment93008> Any reason not to take a const &? How about s/str/version/ in this whole file? We avoid these kinds of abreviations. 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp <https://reviews.apache.org/r/25597/#comment93010> The program will crash if the split is non-numeric, because you'll call .get() on a numify operation, are you intending this to occur? A comment would be nice! 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp <https://reviews.apache.org/r/25597/#comment93013> We use stout/stringify.hpp to convert things to string, what this means is that instead of adding a 'str()' method, you can just add a << operator. Then someone can then output version to a stream, as well as call stringify(version) to get a string representation. Sound good? 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp <https://reviews.apache.org/r/25597/#comment93002> Can you adopt the style of os::Release here please? (1) 'const Version& o', instead of 'const Version &o'. (2) The parens in your == operator are unnecessary. (3) Feel free to just use an 'if' condition to implement the < operator. 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp <https://reviews.apache.org/r/25597/#comment93003> For these two operators, wouldn't the following be easier for people to read and understand? return *this < other || *this == other; Very _direct_ reasoning involed. 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp <https://reviews.apache.org/r/25597/#comment93012> Can you remove all of the _ suffixes for these members? If you look through our codebase, we don't use this pattern. Looks like we don't need to store the 'version' (what you called 'str_' here), since we can re-construct it using major/minor/patch. 3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp <https://reviews.apache.org/r/25597/#comment93004> Unused include? 3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp <https://reviews.apache.org/r/25597/#comment93005> You're not using this? 3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp <https://reviews.apache.org/r/25597/#comment93006> Not using this? - Ben Mahler On Sept. 13, 2014, 12:14 a.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25597/ > ----------------------------------------------------------- > > (Updated Sept. 13, 2014, 12:14 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/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 > >
