Re: Review Request 41593: stout: Added `jsonify` function.

2016-01-05 Thread Benjamin Bannier 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/#review112802
---



3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp (line 107)


Here and in all dtrs below: Given that we don't know whether exceptions are 
enabled for `stream` (this is stout), could we wrap this in a `try` block?, 
e.g., imagine an underlying `fstream` becoming `bad` because there is no space 
left on the device.


- Benjamin Bannier


On Jan. 4, 2016, 11:39 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41593/
> ---
> 
> (Updated Jan. 4, 2016, 11:39 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/README.md 
> a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 98ea47794b3a7c99b3cbd2418ba6e36eb5951259 
> 
> Diff: https://reviews.apache.org/r/41593/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 41593: stout: Added `jsonify` function.

2016-01-05 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/
---

(Updated Jan. 5, 2016, 10:19 a.m.)


Review request for mesos and Benjamin Hindman.


Changes
---

Removed `IsWriteFunction`, added tests.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/Makefile.am 
5ab7bc4966fe32eaddd573a4dbfd997f98b5d481 
  3rdparty/libprocess/3rdparty/stout/README.md 
a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
98ea47794b3a7c99b3cbd2418ba6e36eb5951259 
  3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
62ad461eb228b688f1ceac16cfb003561ed5a806 
  3rdparty/libprocess/3rdparty/stout/tests/jsonify_tests.cpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
bf2a2b8a9f67c6a4cf66b156b9c14fae015a8af0 

Diff: https://reviews.apache.org/r/41593/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 41593: stout: Added `jsonify` function.

2016-01-04 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/
---

(Updated Jan. 4, 2016, 7:28 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
---

Used a more familiar `friend operator<<` declaration.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/README.md 
a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/41593/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 41593: stout: Added `jsonify` function.

2016-01-04 Thread Michael Park 


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, lines 58-63
> > 
> >
> > An implicit cast operator that is non-const is always code smell 
> > because the compiler is what is often implicitly invoking the function! In 
> > this case I know why it's safe for you to do this, but anyone else reading 
> > the code would have no idea. It would be great to have a comment here which 
> > explains why it's safe to `std::move(*this)`, otherwise, this looks 
> > dangerous!
> > 
> > Also, if anyone ever changes this implementation is there anyway to 
> > check this invariant that you have so that they make sure they change this 
> > code too?
> 
> Michael Park wrote:
> Right. I initially had rvalue ref-qualifier on this operator like this 
> `operator std::string() && { ... }`, which makes it so that it is only 
> invocable on a rvalue `JsonifyProxy`. I took it out because I thought maybe 
> it would have been controversial reactions towards it.
> 
> How about we just call `write_` directly instead? It would have been nice 
> to leverage `operator<<` but without the ref-qualifier it's not quite 
> accurate.
> 
> ```cpp
> operator std::string() const {
>   std::ostringstream stream;
>   that.write_();
>   return stream.str();
> }
> ```
> 
> What do you think?

Synced with BenH offline, we agreed to introduce the ref-qualifiers.


- Michael


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/#review111575
---


On Jan. 4, 2016, 7:14 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41593/
> ---
> 
> (Updated Jan. 4, 2016, 7:14 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/README.md 
> a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41593/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 41593: stout: Added `jsonify` function.

2016-01-04 Thread Michael Park 


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 116
> > 
> >
> > Why not s/: Writer/: public Writer/ here and elsewhere?
> 
> Michael Park wrote:
> It's because I don't want `BooleanWriter` to be a `Writer`, but rather 
> simply inherit the common data `std::ostream*` and the deleted 
> constructors/assignment operators. We could do one of the following here:
> 
> (1) Aggregate the `std::ostream*` and delete the constructors/assignment 
> operators in each of the writers rather than factoring it out.
> 
> ```cpp
> class BooleanWriter
> {
> public:
>   BooleanWriter(std::ostream* stream) : stream_(stream), value_(false) {}
>   
>   BooleanWriter(const BooleanWriter&) = delete;
>   BooleanWriter(BooleanWriter&&) = delete;
>   
>   BooleanWriter& operator=(const BooleanWriter&) = delete;
>   BooleanWriter& operator=(BooleanWriter&&) = delete;
>   
>   ~BooleanWriter() { *stream_ << (value_ ? "true" : "false"); }
>   void set(bool value) { value_ = value; }
> 
> private:
>   std::ostream* stream_;
>   bool value_;
> };
> ```
> 
> (2) Aggregate `Writer` instead of private inheritance. In this case the 
> only thing that changes is `s/*stream_/*writer.stream_/`.
> 
> ```cpp
> class BooleanWriter
> {
> public:
>   BooleanWriter(std::ostream* stream) : writer_(stream), value_(false) {}
>   
>   ~BooleanWriter() { *writer.stream_ << (value_ ? "true" : "false"); }
>   void set(bool value) { value_ = value; }
> 
> private:
>   Writer writer_;
>   bool value_;
> };
> ```
> 
> (3) Keep the current "implemented-in-terms-of" semantics of private 
> inheritance.
> 
> ```cpp
> class BooleanWriter : Writer
> {
> public:
>   BooleanWriter(std::ostream* stream) : Writer(stream), value_(false) {}
>   
>   ~BooleanWriter() { *stream_ << (value_ ? "true" : "false"); }
>   void set(bool value) { value_ = value; }
> 
> private:
>   bool value_;
> };
> ```
> 
> Which one do you think is cleanest and/or simplest?

Decided to remove the `Writer` class and repeat the declarations in each of the 
writers.


- Michael


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/#review111575
---


On Jan. 4, 2016, 7:23 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41593/
> ---
> 
> (Updated Jan. 4, 2016, 7:23 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/README.md 
> a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41593/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 41593: stout: Added `jsonify` function.

2016-01-04 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/
---

(Updated Jan. 4, 2016, 7:23 p.m.)


Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/README.md 
a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/41593/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 41593: stout: Added `jsonify` function.

2016-01-04 Thread Michael Park 


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 84
> > 
> >
> > Did you mean to mark this a friend? Is this supposed to be static? Is 
> > this function even used?
> 
> Michael Park wrote:
> Yes, this is intended to be a `friend` because it accesses `write_` which 
> is marked `private`. Currently it's used by `operator std::string()` but with 
> the above changes that could no longer be true. Regardless though, it would 
> be used by expressions such as `std::cout << jsonify(true);`.
> 
> We could use perhaps a more familiar(?) pattern:
> 
> ```cpp
> struct Proxy {
> 
>   /* ... */
>   
>   friend std::ostream& operator<<(std::ostream& stream, Proxy&& that);
> };
> 
> std::ostream& operator<<(std::ostream& stream, Proxy&& that) {
>   that.write_();
>   return stream;
> }
> ```

Used the more familiar pattern.


- Michael


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/#review111575
---


On Jan. 4, 2016, 7:28 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41593/
> ---
> 
> (Updated Jan. 4, 2016, 7:28 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/README.md 
> a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41593/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 41593: stout: Added `jsonify` function.

2016-01-04 Thread Michael Park 


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 120
> > 
> >
> > It seems weird to not be making this virtual ...
> 
> Michael Park wrote:
> Probably because of the fact that public inheritance typically requires a 
> `virtual` destructor (without the `virtual` destructor, deletion through a 
> base pointer doesn't work correctly).
> 
> For example,
> 
> ```cpp
> class Base { ~Base() { ... } };
> class Derived : public Base { ~Derived() { ... } };
> 
> Base* base = new Derived;
> delete base;  // `~Derived` not called!
> ```
> 
> In this case however I'm using private inheritance which is basically 
> aggregation (I've outlined other alternative we can take instead of private 
> inheritance in the previous issue). So `Base* base = new Derived;` doesn't 
> even compile, and therefore deletion through a base pointer can't happen.

Resolved by removing the `Writer` base class.


- Michael


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/#review111575
---


On Jan. 4, 2016, 7:28 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41593/
> ---
> 
> (Updated Jan. 4, 2016, 7:28 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/README.md 
> a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41593/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 41593: stout: Added `jsonify` function.

2016-01-04 Thread Michael Park 


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 167
> > 
> >
> > Is the `int32_t`, `uint32_t`, and `float` overloads necessary?
> 
> Michael Park wrote:
> With only overloads for `int64_t`, `uint64_t`, `double`, passing `0` for 
> example is ambiguous between `int64_t` and `uint64_t`.

Decided to keep all of the overloads to avoid ambiguity.


- Michael


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/#review111575
---


On Jan. 4, 2016, 7:28 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41593/
> ---
> 
> (Updated Jan. 4, 2016, 7:28 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/README.md 
> a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41593/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 41593: stout: Added `jsonify` function.

2016-01-04 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/
---

(Updated Jan. 4, 2016, 11:39 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
---

Moved `void json(const google::protobuf::Message&);` to ``.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/README.md 
a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
98ea47794b3a7c99b3cbd2418ba6e36eb5951259 

Diff: https://reviews.apache.org/r/41593/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 41593: stout: Added `jsonify` function.

2015-12-23 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/
---

(Updated Dec. 23, 2015, 5:57 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
---

Added a comment to explain why `WriterProxy` exists.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/README.md 
a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/41593/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 41593: stout: Added `jsonify` function.

2015-12-23 Thread Michael Park 


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 536
> > 
> >
> > Do you think the comment over `WriterProxy` will be sufficient to 
> > capture this functionality too?

Please take a look at the comment that I've added :)

Also, this has the same problem with `Proxy`'s `operator std::string()` where 
it is a non-const implicit conversion operator. Again, ideally I would declare 
these with ref-qualifiers: `operator BooleanWriter*() &&`.


- Michael


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/#review111575
---


On Dec. 23, 2015, 5:57 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41593/
> ---
> 
> (Updated Dec. 23, 2015, 5:57 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/README.md 
> a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41593/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 41593: stout: Added `jsonify` function.

2015-12-22 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/
---

(Updated Dec. 22, 2015, 8:39 p.m.)


Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/README.md 
a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/41593/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 41593: stout: Added `jsonify` function.

2015-12-22 Thread Michael Park 


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 192
> > 
> >
> > Why not s/type/type_/ here?

I think I was following the guideline of "add the trailing underscore if there 
are conflicts, otherwise use the *nicer* name".

I'm totally on board with using a single naming scheme for member variables 
though :)


- Michael


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/#review111575
---


On Dec. 22, 2015, 8:23 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41593/
> ---
> 
> (Updated Dec. 22, 2015, 8:23 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/README.md 
> a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41593/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 41593: stout: Added `jsonify` function.

2015-12-22 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/
---

(Updated Dec. 22, 2015, 8:42 p.m.)


Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/README.md 
a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/41593/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 41593: stout: Added `jsonify` function.

2015-12-22 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/
---

(Updated Dec. 22, 2015, 8:49 p.m.)


Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/README.md 
a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/41593/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 41593: stout: Added `jsonify` function.

2015-12-22 Thread Michael Park 


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 167
> > 
> >
> > Is the `int32_t`, `uint32_t`, and `float` overloads necessary?

With only overloads for `int64_t`, `uint64_t`, `double`, passing `0` for 
example is ambiguous between `int64_t` and `uint64_t`.


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 624
> > 
> >
> > Since this is in the `internal` namespace, can we 
> > s/jsonify_impl/jsonify/ please?

We sure could. I initially had it named `jsonify` and had it returning `Proxy`, 
but when I changed it to `std::function`, I thought `jsonify_impl` would be a 
better name for it.


- Michael


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/#review111575
---


On Dec. 22, 2015, 8:23 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41593/
> ---
> 
> (Updated Dec. 22, 2015, 8:23 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/README.md 
> a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41593/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 41593: stout: Added `jsonify` function.

2015-12-22 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/
---

(Updated Dec. 22, 2015, 8:43 p.m.)


Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/README.md 
a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/41593/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 41593: stout: Added `jsonify` function.

2015-12-22 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/
---

(Updated Dec. 23, 2015, 3:56 a.m.)


Review request for mesos and Benjamin Hindman.


Changes
---

Removed the use of macros in the `json` definition for 
`google::protobuf::Message`.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/README.md 
a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/41593/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 41593: stout: Added `jsonify` function.

2015-12-22 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/
---

(Updated Dec. 22, 2015, 8:23 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
---

Addressed some of Ben's comments.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/README.md 
a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
  3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/41593/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 41593: stout: Added `jsonify` function.

2015-12-22 Thread Michael Park 


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, lines 58-63
> > 
> >
> > An implicit cast operator that is non-const is always code smell 
> > because the compiler is what is often implicitly invoking the function! In 
> > this case I know why it's safe for you to do this, but anyone else reading 
> > the code would have no idea. It would be great to have a comment here which 
> > explains why it's safe to `std::move(*this)`, otherwise, this looks 
> > dangerous!
> > 
> > Also, if anyone ever changes this implementation is there anyway to 
> > check this invariant that you have so that they make sure they change this 
> > code too?

Right. I initially had rvalue ref-qualifier on this operator like this 
`operator std::string() && { ... }`, which makes it so that it is only 
invocable on a rvalue `JsonifyProxy`. I took it out because I thought maybe it 
would have been controversial reactions towards it.

How about we just call `write_` directly instead? It would have been nice to 
leverage `operator<<` but without the ref-qualifier it's not quite accurate.

```cpp
operator std::string() const {
  std::ostringstream stream;
  that.write_();
  return stream.str();
}
```

What do you think?


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 84
> > 
> >
> > Did you mean to mark this a friend? Is this supposed to be static? Is 
> > this function even used?

Yes, this is intended to be a `friend` because it accesses `write_` which is 
marked `private`. Currently it's used by `operator std::string()` but with the 
above changes that could no longer be true. Regardless though, it would be used 
by expressions such as `std::cout << jsonify(true);`.

We could use perhaps a more familiar(?) pattern:

```cpp
struct Proxy {

  /* ... */
  
  friend std::ostream& operator<<(std::ostream& stream, Proxy&& that);
};

std::ostream& operator<<(std::ostream& stream, Proxy&& that) {
  that.write_();
  return stream;
}
```


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 116
> > 
> >
> > Why not s/: Writer/: public Writer/ here and elsewhere?

It's because I don't want `BooleanWriter` to be a `Writer`, but rather simply 
inherit the common data `std::ostream*` and the deleted constructors/assignment 
operators. We could do one of the following here:

(1) Aggregate the `std::ostream*` and delete the constructors/assignment 
operators in each of the writers rather than factoring it out.

```cpp
class BooleanWriter
{
public:
  BooleanWriter(std::ostream* stream) : stream_(stream), value_(false) {}
  
  BooleanWriter(const BooleanWriter&) = delete;
  BooleanWriter(BooleanWriter&&) = delete;
  
  BooleanWriter& operator=(const BooleanWriter&) = delete;
  BooleanWriter& operator=(BooleanWriter&&) = delete;
  
  ~BooleanWriter() { *stream_ << (value_ ? "true" : "false"); }
  void set(bool value) { value_ = value; }

private:
  std::ostream* stream_;
  bool value_;
};
```

(2) Aggregate `Writer` instead of private inheritance. In this case the only 
thing that changes is `s/*stream_/*writer.stream_/`.

```cpp
class BooleanWriter
{
public:
  BooleanWriter(std::ostream* stream) : writer_(stream), value_(false) {}
  
  ~BooleanWriter() { *writer.stream_ << (value_ ? "true" : "false"); }
  void set(bool value) { value_ = value; }

private:
  Writer writer_;
  bool value_;
};
```

(3) Keep the current "implemented-in-terms-of" semantics of private inheritance.

```cpp
class BooleanWriter : Writer
{
public:
  BooleanWriter(std::ostream* stream) : Writer(stream), value_(false) {}
  
  ~BooleanWriter() { *stream_ << (value_ ? "true" : "false"); }
  void set(bool value) { value_ = value; }

private:
  bool value_;
};
```

Which one do you think is cleanest and/or simplest?


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 120
> > 
> >
> > It seems weird to not be making this virtual ...

Probably because of the fact that public inheritance typically requires a 
`virtual` destructor (without the `virtual` destructor, deletion through a base 
pointer doesn't work correctly).

For example,

```cpp
class Base { ~Base() { ... } };
class Derived : public Base { ~Derived() { ... } };

Base* base = new Derived;
delete base;  // `~Derived` not called!
```

In this case however I'm using private inheritance which is basically 
aggregation (I've outlined other alternative we can take instead of private 
inheritance in the previous

Re: Review Request 41593: stout: Added `jsonify` function.

2015-12-20 Thread Michael Park 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/#review111438
---



3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp (line 507)


I still need to write the comment for this. I'll update this review once I 
have done so, but other parts can still be reviewed.


- Michael Park


On Dec. 21, 2015, 4:55 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41593/
> ---
> 
> (Updated Dec. 21, 2015, 4:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/README.md 
> a2a3dc6120b10c8fe82088b9f0ebecfa5642f945 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe 
>   3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41593/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>