mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Clemmer" <clemmer.alexan...@gmail.com>
Subject Re: Review Request 39803: Windows: Implemented stout/os/stat.hpp`.
Date Thu, 14 Jan 2016 22:17:23 GMT


> 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
`->` operator.

(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 (4).


- Alex


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


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
PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp PRE-CREATION

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message