mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Bannier" <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 39803: Windows: Implemented stout/os/stat.hpp`.
Date Tue, 17 Nov 2015 14:20:26 GMT


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 107
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113684#file1113684line107>
> >
> >     Is the enum+switch+unreachable a common pattern in stout? It seems that choosing
to use an enume for follw, instead of a bool made this code much more complex.
> >     
> >     Also, this is the only use of UNREACHABLE(), so you could remove the #include
if you reworked the function.
> >     
> >     Perhaps size() is an existing API in stout and you have no choice about its
prototype? In that case, are you sure that your version of enum FollowSymlink is exactly the
same as the one used in the Linux version. 
> >     
> >     You would hate for someone on the Linux side to take a dependency on the order
of the items inside the enum and find that you had changed that order.
> 
> Joseph Wu wrote:
>     Yes, `UNREACHABLE` is commonly used in enum-switch statements.  
>     
>     There is another pattern of excluding a `default` and adding a blurb like:
>     ```
>     // NOTE: By not setting a default we leverage the compiler
>     // errors when the enumeration is augmented to find all
>     // the cases we need to provide.
>     ```
>     (We do end up using `UNREACHABLE` anyway in some of those cases too.)

In light of https://issues.apache.org/jira/browse/MESOS-3754 I went and checked how often
we actually add a `default` branch when we `switch` over enum values. This is the `clang-query`
I used (which probably misses some cases of fall-through into `default`):

    match switchStmt(has(declRefExpr(hasType(enumDecl()))),
                     has(compoundStmt(has(defaultStmt().bind("default")))))

After checking all stout tests, their includes, and similarly all libprocess `.cpp` files
I could only find a single such case in `os::stat::size`.

We *do use* `default` branches when switching over int values though; here the domain makes
it impractical to enumerate all values ;)


- Benjamin


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


On Nov. 16, 2015, 9:14 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39803/
> -----------------------------------------------------------
> 
> (Updated Nov. 16, 2015, 9:14 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 a8c35c086ecae21701f6a720f25231c1b0d4e329

>   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/windows/stat.hpp 675b2e712358a55b3580026936890eaf80e5af71

>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 1a7037d64afeedc340258c92067e95d1d3caa027

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