----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58707/#review173649 -----------------------------------------------------------
Fix it, then Ship it! 3rdparty/stout/include/stout/version.hpp Lines 77 (patched) <https://reviews.apache.org/r/58707/#comment246654> See below? 3rdparty/stout/include/stout/version.hpp Lines 102 (patched) <https://reviews.apache.org/r/58707/#comment246658> I guess this breaks for the negative case? E.g. 1.0.0--1.-1 We'll treat the -1's as large numbers based on the numify test case you showed earlier? Rather than being invalid (no negative allowed) or non-numeric (due to the hyphen). Not clear which one is correct from the spec AFAICT, unfortunately :( 3rdparty/stout/include/stout/version.hpp Lines 104-111 (patched) <https://reviews.apache.org/r/58707/#comment246669> Did you consider making this an optional validation of validateIdentifier? That would allow us to do invariant CHECKs in the constructor, see my other comment. 3rdparty/stout/include/stout/version.hpp Line 62 (original), 131 (patched) <https://reviews.apache.org/r/58707/#comment246662> Do we need a TODO here to reject negatives? Or do you want to just implement that now? 3rdparty/stout/include/stout/version.hpp Line 66 (original), 137-144 (patched) <https://reviews.apache.org/r/58707/#comment246668> I guess we could do this in a separate patch, to make it clearer that we're making a change to the existing behavior rather than only adding build and prerelease label support? 3rdparty/stout/include/stout/version.hpp Lines 73-76 (original), 156-167 (patched) <https://reviews.apache.org/r/58707/#comment246667> It would be nice to catch errors early here and do the CHECKs in the constructor in case someone tries to construct a malformed version: ``` { foreach (const std::string& identifier, prerelease) { CHECK_NONE(validateIdentifier(identifier, true)); // disallow_leading_zeros=true } foreach (const std::string& identifier, build) { CHECK_NONE(validateIdentifier(identifier)); } } ``` Separately from your change, it would be nice to also use unsigned integers to avoid needing to validate against negative numbers, but we can do this in a separate patch. 3rdparty/stout/include/stout/version.hpp Lines 243-264 (patched) <https://reviews.apache.org/r/58707/#comment246666> Should we keep the notion of "other" here rather than "1" and "2"? I found the nesting here to make this a little hard to follow, maybe we can use a flat list of cases to make this easier to read? ``` if (identifier.isSome() && other_identifier.isSome()) { // Both are numeric. if (identifier.get() != other_identifier.get()) { return identifier.get() < other_identifier.get(); } } else if (identifier.isSome()) { // If `this` identifier is numeric but `other` is not, `this < other`. return true; } else if (other_identifier.isSome()) { // If `other` identifier is numeric but `this` is not, `other < this`. return false; } else { // Neither identifier is numeric, so compare them via ASCII sort. if (prerelease.at(i) != other.prerelease.at(i)) { return prerelease.at(i) < other.prerelease.at(i); } } ``` 3rdparty/stout/tests/version_tests.cpp Lines 90-91 (original), 149-152 (patched) <https://reviews.apache.org/r/58707/#comment246670> Oh.. I see that negatives are being caught correctly, I didn't realize when reading the code that we were catching them due to the pre-release label parsing. Hm.. maybe we need a little note about that in the major,minor,patch numification loop? - Benjamin Mahler On May 2, 2017, 8 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58707/ > ----------------------------------------------------------- > > (Updated May 2, 2017, 8 p.m.) > > > Review request for mesos, Benjamin Mahler and Kapil Arya. > > > Bugs: MESOS-1987 > https://issues.apache.org/jira/browse/MESOS-1987 > > > Repository: mesos > > > Description > ------- > > Previously, Stout's `Version` abstraction only supported a subset of > Semver: version numbers with three numeric components (an optional > trailing "label" with a leading hyphen was supported but ignored). > > This commit adds support for SemVer 2.0.0, which defines two additional > optional fields: a "prerelease label" and a "build metadata label", > e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of > dot-separated identifiers. > > > Diffs > ----- > > 3rdparty/stout/include/stout/version.hpp > 7717c85b95d29cefe8f19f3cada4b7402d4d446f > 3rdparty/stout/tests/version_tests.cpp > 925f3833fd8b1732c35663df3e63c161261e9bff > > > Diff: https://reviews.apache.org/r/58707/diff/5/ > > > Testing > ------- > > `make check` > > > Thanks, > > Neil Conway > >
