> 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? > > 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? > > Kapil Arya wrote: > 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 :).
Definitely! Since the first use case is replacing os::Release, we can leave a TODO on Version to deal with more than 3 components. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25597/#review53370 ----------------------------------------------------------- On Sept. 16, 2014, 8:40 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, 8:40 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 > >
