> On Sept. 15, 2014, 6:46 p.m., Ben Mahler wrote: > > 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!
Thanks for following up! Mind adding your name to the TODO in os.hpp? > On Sept. 15, 2014, 6:46 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, lines 86-89 > > <https://reviews.apache.org/r/25597/diff/3/?file=688423#file688423line86> > > > > 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. > > Kapil Arya wrote: > The public methods are named major(), minor(), and patch() and that's why > the _ suffix for the data members. What is the proper way to handle this > situation? Of course, one possibility is to rename major/minor/patch methods > with get{Major,Minor,Patch}(). Any suggestions? That makes sense, the suffixes are good with what you have now. If you were to have a 'parse' method (per my comment above), then these could become const member variables and the accessor methods would be unneeded. :) > On Sept. 15, 2014, 6:46 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, lines 28-29 > > <https://reviews.apache.org/r/25597/diff/3/?file=688423#file688423line28> > > > > Any reason not to take a const &? > > > > How about s/str/version/ in this whole file? We avoid these kinds of > > abreviations. > > Dominic Hamon wrote: > i was going to make the same comment, but given this copies directly into > a member there's actually no benefit to taking the const reference. See the > accepted answer on > http://stackoverflow.com/questions/21605579/how-true-is-want-speed-pass-by-value > for why. It's a subtle detail, and actually comes down to style to, but we > might want to get used to more cases where we pass by (possibly const) value > instead of reference. > > Kapil Arya wrote: > So, if I understand correctly, a pass-by-value is ok here? If not I can > change it to a const &. Thanks for the link Dominic, I would say let's stick with const & for consistency with the rest of the code. > On Sept. 15, 2014, 6:46 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 25 > > <https://reviews.apache.org/r/25597/diff/3/?file=688423#file688423line25> > > > > 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. > > Kapil Arya wrote: > Thanks for pointing it out. I will add a TODO for now and in a later > patch, we can consolidate struct Release and Version. Thank you! > On Sept. 15, 2014, 6:46 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, lines 32-34 > > <https://reviews.apache.org/r/25597/diff/3/?file=688423#file688423line32> > > > > 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! > > Kapil Arya wrote: > Good catch! The question now is how to handle the situation where a > string such as "1.a.2" is passed on? Should it be considered an error or > should it be interpreted as "1.0.0"? If it should be considered an error, > how should be change the contructor to reflect that? You can add a 'parse' method akin to what was done in duration.hpp: ``` static Try<Version> parse(const std::string& version); ``` This makes the error explicit, and we no longer need the (major(), minor(), patch()) accessor methods, since the member variables would be const. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25597/#review53370 ----------------------------------------------------------- On Sept. 15, 2014, 10:22 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25597/ > ----------------------------------------------------------- > > (Updated Sept. 15, 2014, 10:22 p.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 > >
