> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, lines 58-63
> > <https://reviews.apache.org/r/41593/diff/2/?file=1173648#file1173648line58>
> >
> >     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_(&stream);
  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
> > <https://reviews.apache.org/r/41593/diff/2/?file=1173648#file1173648line84>
> >
> >     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_(&stream);
  return stream;
}
```


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, line 116
> > <https://reviews.apache.org/r/41593/diff/2/?file=1173648#file1173648line116>
> >
> >     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
> > <https://reviews.apache.org/r/41593/diff/2/?file=1173648#file1173648line120>
> >
> >     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 issue). So `Base* base = new Derived;` doesn't even 
compile, and therefore deletion through a base pointer can't happen.


> On Dec. 22, 2015, 4:52 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, lines 49-51
> > <https://reviews.apache.org/r/41593/diff/2/?file=1173648#file1173648line49>
> >
> >     We should be able to make this work. Why can't we:
> >     
> >     ```
> >     namespace JSON { class Proxy; }
> >     
> >     template <typename T>
> >     JSON::Proxy jsonify(const T&);
> >     
> >     namespace JSON {
> >     
> >     class Proxy
> >     {
> >     ...
> >     private:
> >       template <typename T>
> >       friend Proxy (::jsonify)(const T&);
> >     };
> >     
> >     } // namespace JSON {
> >     
> >     template <typename T>
> >     JSON::Proxy jsonify(const T&)
> >     {
> >        ...
> >     }
> >     ```
> >     
> >     I think the crux is putting the parens around `::jsonify` in the friend 
> > declaration.

Nice! Thanks for this. Gotta love magic parens :(


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

Reply via email to