----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43561/#review136543 -----------------------------------------------------------
Hi Klaus sorry for the delay. This piece of code is very old in Mesos and has some significant technical debt. We should have used a parsing library here but at the time we didn't have <regex> because we hadn't moved to C++11 yet. From what I can tell, regex needs GCC 4.9+ to work correctly in the std:: namespace. There appears to be a tr1 regex library. Can you do some investigation on whether we can pull in a regex library? I would suggest mailing the dev@ list to see if anyone has looked into this before. Let me know what you find, I'm hesitant to make the parsing even more complicated and hacky here. Until then, I would suggest splitting apart the tests here for each value type, and show all accepted formats (there are a lot that we'll accept currently that we don't show in the tests!). src/common/values.cpp (lines 651 - 654) <https://reviews.apache.org/r/43561/#comment201615> Can you pull this cleanup into a separate patch? Note that you don't need the variable: ``` foreach (const string& token, sstrings::tokenize(temp, "{},\n")) { set->add_item(token); } ``` src/tests/values_tests.cpp (lines 39 - 72) <https://reviews.apache.org/r/43561/#comment201614> It would be great to split apart this test into the respective value types, e.g. ``` ValuesTest.ParseScalar ValuesTest.ParseRanges ValuesTest.ParseSet ``` With more test cases that show what is currently accepted and what is not. There are a lot of other formats currently accepted from what I can tell (see my comment below). src/tests/values_tests.cpp (line 210) <https://reviews.apache.org/r/43561/#comment201609> If I understand correctly, the following are currently accepted way to specify 1-4: `[[1-4]]` `[1-2]\n[3-4]` `[1-2],[3-4]` I'm ok with rejecting these in favor of the following canonical formats: `[1-2,3-4]` Although it doesn't have to be coalesced. However, we should notify the user mailing list that we're moving towards more strict parsing of ranges if you're going to change what we accept. Ideally, we could pull in a regex or parsing expression grammar library to more formally define our formats. The current parsing code is a mess. - Benjamin Mahler On March 22, 2016, 1:22 p.m., Klaus Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43561/ > ----------------------------------------------------------- > > (Updated March 22, 2016, 1:22 p.m.) > > > Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Joris Van > Remoortere. > > > Bugs: MESOS-4627 > https://issues.apache.org/jira/browse/MESOS-4627 > > > Repository: mesos > > > Description > ------- > > Improve Ranges parsing to handle single values. > > > Diffs > ----- > > src/common/values.cpp 9f3c0b9930bd3dd9d84d97119419825862f531c3 > src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c > src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 > > Diff: https://reviews.apache.org/r/43561/diff/ > > > Testing > ------- > > make > make check GTEST_FILTER=~"*" > ./src/mesos-test > > > Thanks, > > Klaus Ma > >
