----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22825/#review46512 -----------------------------------------------------------
Unit tests look good. My other concerns were not addressed though. Reiterating here. 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp <https://reviews.apache.org/r/22825/#comment81938> Please comment that the string is split each time at the first character that matches any of the characters specified in delims. Otherwise, delims might be construed as a multi-character delimiter string. 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp <https://reviews.apache.org/r/22825/#comment81939> (Still not fixed.) Why iterate through the rest of the string with another find_first_of() call if n==1? Check for (n==1) before calling find_first_of, and you can potentially save a lot of character comparisons. Try: if (n==1) { tokens.push_back(...); break; } next = s.find_first_of(...); if (next == npos) { tokens.push_back(...); break; } ... Or: while (n > 1) {...} with a special post-condition for n==1. - Adam B On June 24, 2014, 2:07 a.m., Isabel Jimenez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22825/ > ----------------------------------------------------------- > > (Updated June 24, 2014, 2:07 a.m.) > > > Review request for mesos, Benjamin Hindman, Dominic Hamon, and Till Toenshoff. > > > Bugs: MESOS-1522 > https://issues.apache.org/jira/browse/MESOS-1522 > > > Repository: mesos-git > > > Description > ------- > > SplitN function is stout/strings so we can choose how many tokens we want to > retreive > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/strings.hpp 08428b8 > 3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp c83156e > > Diff: https://reviews.apache.org/r/22825/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Isabel Jimenez > >
