----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32163/#review76897 -----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment124590> 1. I'd rather forward-declare the class, not the inline function. Matter of taste, yes. "inline" without function body just seems weird. More importantly, reading the whole function body and the function comments (see below) first provides a nice setup for understanding the class, which is the complicated part here, not the other way around. 2. The function needs a comment about what it does more than the class, because it is part of the API. (The class still needs one, too.) 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment124604> This is still pretty vague. What gets compared to what? Which side is the value in the comparator on? 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment124588> "for which checks" -> "for which it checks" 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment124589> // see bool contains(const Value&, const Value&) -> // See 'bool contains(const Value&, const Value&)'. 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment124608> I am guessing (finding out later) that this is testing if 'object' is contained in 'value'? Correct? Why not write the purpose of this operator down? 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment124611> The text names the comparison in the opposite direction of the code. Easier to read if they parallel each other. However, this is not really a line that needs any comment. This particular part of the code is entirely self-explanatory and any comment is redundant. Higer up, describing the whole operator, is where I would put high level information about what this algorithm tries to do. 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment124612> Need to have or are guaranteed to have or both? I believe both. 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment124615> Redundant. That's exactly what the code says in one line, too. 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment124616> Also redundant. Looks good. Just small matters of taste marked in this review. First pass, have not looked at the tests yet. - Bernd Mathiske On March 18, 2015, 7:32 a.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32163/ > ----------------------------------------------------------- > > (Updated March 18, 2015, 7:32 a.m.) > > > Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, > Niklas Nielsen, and Till Toenshoff. > > > Bugs: MESOS-2510 > https://issues.apache.org/jira/browse/MESOS-2510 > > > Repository: mesos > > > Description > ------- > > Adds a function which allows to perform comparison tests on subsets of json > blobs. i.e. > > ```cpp > JSON::Value expected = JSON::parse( > "{" > " \"key\" : true" > "}").get(); > > // Returned json: > // { > // "uptime" : 45234.123, > // "key" : true > // } > JSON::Value actual = bar(); > > // I'm only interested on the "key" entry and ignore the rest. > EXPECT_TRUE(contains(actual, expected)); > ``` > > Increasing readability for tests that include json. > > For more information on the reason of why this patch is needed, please check > the JIRA entry. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp > 334c898906018be6e663f53815abbe047806b95c > 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp > f60d1bbe60f2e2b6460c06bba98e8b85ebb6a3f9 > > Diff: https://reviews.apache.org/r/32163/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
