> On June 2, 2015, 9:10 a.m., Benjamin Hindman wrote: > > src/common/http.cpp, lines 212-213 > > <https://reviews.apache.org/r/34687/diff/4/?file=975084#file975084line212> > > > > Please see formatting for braces: > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Braced_Initializer_List_Format > > > > tl;dr; Use braced-initializer list construction like a constructor. > > > > JSON::Protobuf protobuf{masterInfo}; > > > > Please fix throughout this review. > > > > But this brings me to a second question, why use braced-initializer > > list here at all when you're just calling a normal constructor (i.e., not a > > constructor that takes an std::initializer_list or an implicit constructor > > that just sets the fields of the object). > > > > Finally, this can be simplified: > > > > JSON::Object json(JSON::Protobuf(masterInfo)); > > Marco Massenzio wrote: > sorry - this was a remnant from some trial code I'd run when playing > around with JSON::Protobuf (just to see what came out). > Fixed. > > I'm hoping I can bring cpplint to detect all this stuff.
sorry to break your heart, mate - but your code causes the dreaded "most vexing parse"... other variants fail too. I'm sure we can come with a clever one-liner constructor, but in the spirit of the C++ Committee (guidelines](https://isocpp.org/std/library-design-guidelines) of "don't be clever", would you object to a (readable): ``` JSON::Protobuf protobuf(masterInfo); JSON::Object json(protobuf); ``` not too "clever", but not terribly "stupid" either :) - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review86176 ----------------------------------------------------------- On May 31, 2015, 2:58 a.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34687/ > ----------------------------------------------------------- > > (Updated May 31, 2015, 2:58 a.m.) > > > Review request for mesos, haosdent huang and Niklas Nielsen. > > > Bugs: MESOS-2340 > https://issues.apache.org/jira/browse/MESOS-2340 > > > Repository: mesos > > > Description > ------- > > Jira: MESOS-2340 > > This is a preliminary step to enabling JSON API > for Master Discovery via Zookeeper. > > We currently save the MasterInfo PB to ZK > serializing directly the binary data, so that > for clients to retrieve that information, they > need to either link up with libmesos or > obtain the PB definition (in mesos/mesos.proto). > > This change only provides the (de)serialization > utility methods and associated tests. > > > Diffs > ----- > > src/Makefile.am e7281ac6667562a27b0427b0ba7f41de98702928 > src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 > src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a > src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 > src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 > src/tests/common/parse_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/34687/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Marco Massenzio > >
