> 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! > > 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? > > Ben Mahler wrote: > 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. > > Kapil Arya wrote: > There is one trouble with making major/minor as const fields. Glibc > defines major and minor as macros and that breaks the compilation when these > fields are initialized in a contructor as ": major(majorVersion), > minor(minorVersion), patch(patchVersion)". The preprocessor expands them to > gnu_dev_major and gnu_dev_minor respectively. > > The alternative solution is to go back to accessor functions and mark > these fields as private. Another alternative is to rename the const fields as > majorVersion, minorVersion, and patchVersion. > > Is there any preference here? > > Ben Mahler wrote: > I see, seems a bit tricky to skirt around the macro issue: > > ``` > // This caller is ok? > int major = version.major(); > > // The constructor argument is ok? > Version::Version(int major, int minor, int patch) {} > ``` > > What if we remove the accessors, and make them const private as 'major_', > 'minor_', 'patch_'? > > I'm curious why we'd need access to the version components, rather than > just using the comparison operators or the stringify operator. > I could also envision some compatibility operators to ensure that it's > the same major version if >= 1.0, or same minor version if < 1.0. > > How about we hide them for now and defer the decision for later should > the need arise?
I can't think of any reason for explicitly accessing the version components so I agree that we should hide them for now. Along the same lines, the next question (came up with discussion with Bernd) is that why shouldn't we have arbitrary number of components rather than just having major, minor, and patch? Shouldn't we just keep a vector of components that is allowed to grow to arbitrary lengths? We certainly see more than just major/minor/patch in Linux kernel version for example. The counter argument is that a lot of systems (including Linux) are trying to get to a simpler versioning scheme with less components :). - Kapil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25597/#review53370 ----------------------------------------------------------- On Sept. 16, 2014, 2:48 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25597/ > ----------------------------------------------------------- > > (Updated Sept. 16, 2014, 2:48 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 > >
