> On Jan. 13, 2016, 6:01 a.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp, lines 37-40
> > <https://reviews.apache.org/r/39803/diff/8/?file=1192872#file1192872line37>
> >
> >     We typically don't use typedefs in Mesos. Can we just use 
> > `std::shared_ptr<void>`?
> Alex Clemmer wrote:
>     Per our Slack conversation: (1) I'm all for judicious use of `typedef`, 
> and in this case I would really prefer to keep `void *` out of the codebase 
> where possible. In Windows we expect to make prodigious use of 
> `shared_handle`, and it is (IMHO) dramatically clearer than 
> `shared_ptr<void>`, so I'd humbly like to suggest we take an exception here. 
> And (2) regardless, we typedef all over the place in `windows.hpp` anyway.
>     I'm really hoping peopl will agree that this is ok after all. :)

mpark and I have gone back and forth over this a few times today. After some 
consideration, there are a few options on the table: (1) write a SharedHandle 
class, (2) inherit from `shared_ptr`, (3) use the `typedef` above, (4) use 
`shared_ptr<void>`. I think (2) is the best of these approaches, and if I can't 
have that, I would like (3). Let's go through each in turn.

(1) A `SharedHandle` class might look like this:

// An RAII `HANDLE` based on `shared_ptr`.
struct SharedHandle {
  template <typename Deleter>
  SharedHandle(HANDLE handle, Deleter deleter) : handle_(handle, deleter) {}
  std::shared_ptr<void> handle_;

The benefits are that it restricts the `shared_ptr` constructor to just the 
case we want (_i.e._, we always want to pass the close function in, like 
`SharedHandle h(handle, FindClose)`, and it fully covers the zoo of functions 
that close different types of `HANDLE`. The downside is that none of the 
`shared_ptr` API is taken with us -- we don't get to use stuff like the `->` 

(2) Having `SharedHandle` inherit from `shared_ptr` and pulling in the APIs we 
want using declarations, mitigates the problem of `SharedHandle` not having the 
`shared_ptr` API. This is kind of annoying, though, and it's a bit overkill. 
The `shared_ptr` API does not seem so dangerous, and this is the core use case 
of its API. IMHO this danger does not warrant maintaining this complexity. In 
fact, I'd rather have the simplicity of `shared_ptr<void>` than this option.

(3) and (4) Having a typedef `shared_handle` is a natural extension of the 
Windows API's `HANDLE` which itself is a typedef of `void*`. Passing around a 
`shared_ptr<void>` introduces the user to this implementation detail of the 
`HANDLE`, that is simply unnecessary. It is also by far the simplest of the 
solutions I consider "good". And, since there is precedent for this choice in 
the Windows API, and since there is precedent even here in `windows.hpp`, I 
propose we just use option (3). If I can't have (3) I prefer the simplicity of 

- Alex

This is an automatically generated e-mail. To reply, visit:

On Jan. 14, 2016, 10:08 a.m., Alex Clemmer wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39803/
> -----------------------------------------------------------
> (Updated Jan. 14, 2016, 10:08 a.m.)
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> Repository: mesos
> Description
> -------
> Windows: Implemented stout/os/stat.hpp`.
> Diffs
> -----
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> ec851dcb08d938defeb6066810011e003d594b29 
> 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp
> 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/stat.hpp 
> ffe074830ef90f492990bf695e686c885b9a155c 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp 
> 5b38b9af654d7d1c574f0cc573083b66693ced1d 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 27edf1b4f8647a2720f2962eafa110d694b3baed 
> Diff: https://reviews.apache.org/r/39803/diff/
> Testing
> -------
> `make check` from autotools on Ubuntu 15.
> `make check` from CMake on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> Thanks,
> Alex Clemmer

Reply via email to