----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49223/#review143028 -----------------------------------------------------------
Sorry for the delay, mostly some bugs around use of tokenize rather than split, and some cleanup suggestions. Be sure to update the two values.cpp files in the same way, based on the comments. src/common/values.cpp (lines 577 - 583) <https://reviews.apache.org/r/49223/#comment208706> This can just be: ``` // Remove all spaces. string temp = strings::replace(text, " ", ""); ``` Feel free to simplify this in another patch. src/common/values.cpp (lines 589 - 594) <https://reviews.apache.org/r/49223/#comment208708> Do we still need this? Seems to me that we want to do the following: ``` // Ranges. E.g. [1-2,4-5] if (strings::startsWith(temp, "[")) { if (!strings::endsWith(temp, "]")) { return Error("Missing a closing bracket"); } ... } if (strings::startsWith(temp, "{")) { if (!strings::endsWith(temp, "}")) { return Error("Missing a closing brace"); } .. } ``` Also, why is this looking at parentheses? I only see brackets (ranges) and braces (set) being used. src/common/values.cpp (line 604) <https://reviews.apache.org/r/49223/#comment208705> Looks like you want a strings::split here? strings::tokenize will ignore empty fields: ``` // Using tokenize makes these two equivalent. [1-2,,,,5-6] [1-2,5-6] ``` Can you add a test that we don't allow the extra commas since it's not the canonical format? Also, why did you keep tokenizing on newlines? AFAICT they are not part of the canonical format? src/common/values.cpp (lines 604 - 606) <https://reviews.apache.org/r/49223/#comment208714> You don't need the temporary variable? ``` foreach (const string& token, strings::split(temp, ",")) { ... } ``` src/common/values.cpp (line 607) <https://reviews.apache.org/r/49223/#comment208716> Seems like we need strings::split here to avoid accepting ranges like `--1------2-` src/common/values.cpp (line 609) <https://reviews.apache.org/r/49223/#comment208715> This message seems confusing. How about: Invalid range '1 to 2'; expected format is 'begin-end' src/common/values.cpp (lines 614 - 615) <https://reviews.apache.org/r/49223/#comment208717> We use `numify<uint64_t>` now, mind updating this and removing the include? Feel free to post a cleanup patch in front of this one. src/common/values.cpp (line 635) <https://reviews.apache.org/r/49223/#comment208712> Ditto comments from Ranges here. (tokenize and newline) src/common/values.cpp (lines 635 - 636) <https://reviews.apache.org/r/49223/#comment208713> You don't need the temporary variable? ``` foreach (const string& token, strings::split(temp, ",")) { ... } ``` src/common/values.cpp (lines 637 - 638) <https://reviews.apache.org/r/49223/#comment208719> Please document in the description that this isn't a complete validation against the canonical format because we aren't validating text yet. The current summary of this commit seems to suggest that after applying this patch we'll **only** accept the canonical format (but that's not quite true). src/common/values.cpp (line 649) <https://reviews.apache.org/r/49223/#comment208718> Ditto about numify<double> here (but please do it in a separate patch in front of this one). src/tests/values_tests.cpp (lines 88 - 91) <https://reviews.apache.org/r/49223/#comment208711> Can we also test the braces? ``` // Only a single pair of braces are allowed. EXPECT_ERROR(parse("{a,b}{c,d}")); EXPECT_ERROR(parse("{a,b},{c,d}")); EXPECT_ERROR(parse("{a,b,{c,d}}")); ``` Since this isn't rejected just yet, please update the description to reflect that this patch isn't doing a complete validation against the canonical format. We can do the following to make it clear to the reader of the test that we don't fully validate yet: ``` // TODO(klaus1982): Only a single pair of braces should be allowed, // however we do not validate the set entries as conforming to the // Values.Text format yet. // // EXPECT_ERROR(parse("{a,b}{c,d}")); // EXPECT_ERROR(parse("{a,b},{c,d}")); // EXPECT_ERROR(parse("{a,b,{c,d}}")); ``` - Benjamin Mahler On July 16, 2016, 2:39 a.m., Klaus Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49223/ > ----------------------------------------------------------- > > (Updated July 16, 2016, 2:39 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-5739 > https://issues.apache.org/jira/browse/MESOS-5739 > > > Repository: mesos > > > Description > ------- > > Fixed Value parsing code to only accept the canonical formats. > > > Diffs > ----- > > src/common/values.cpp 7b8a8ea00bd4bd1a82c33be4438c26b2731b1dd8 > src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba > src/v1/values.cpp 3e0f739b69e680f9eb29ed36d4501c393758c861 > > Diff: https://reviews.apache.org/r/49223/diff/ > > > Testing > ------- > > make && make check > > > Thanks, > > Klaus Ma > >
