> On March 18, 2015, 6:28 p.m., Bernd Mathiske wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp, line 355 > > <https://reviews.apache.org/r/32163/diff/4/?file=898939#file898939line355> > > > > 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.)
1. I agree, however the definition of the function creates an instance of the class and one needs a type definition when creating an instance, so it cannot be done. 2. The documentation is in the definition of the function some lines below, I will add a "see" comment. > On March 18, 2015, 6:28 p.m., Bernd Mathiske wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp, line 419 > > <https://reviews.apache.org/r/32163/diff/4/?file=898939#file898939line419> > > > > Also redundant. Not relevant anymore with code changes. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32163/#review76897 ----------------------------------------------------------- On March 18, 2015, 3:32 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32163/ > ----------------------------------------------------------- > > (Updated March 18, 2015, 3:32 p.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 > >
