> On Sept. 15, 2014, 2: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!
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? > On Sept. 15, 2014, 2:46 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, lines 75-83 > > <https://reviews.apache.org/r/25597/diff/3/?file=688423#file688423line75> > > > > 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. We can do that. The only reason for the current version was to avoid additional comparisons :). > On Sept. 15, 2014, 2: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. Thanks for pointing it out. I will add a TODO for now and in a later patch, we can consolidate struct Release and Version. > On Sept. 15, 2014, 2: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. So, if I understand correctly, a pass-by-value is ok here? If not I can change it to a const &. - Kapil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25597/#review53370 ----------------------------------------------------------- On Sept. 15, 2014, 6: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, 6: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 > >
