> 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?
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?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25597/#review53370
-----------------------------------------------------------
On Sept. 16, 2014, 6: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, 6: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
>
>
