> 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
> 
>

Reply via email to