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 Mon, 11 Jan 2016 20:24:46 GMT


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp, line
61
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113683#file1113683line61>
> >
> >     Recommend opening a work item for Windows team to add this API. Who knows what
version of Windows would actually ship it, but the world would be a slightly better place.
> >     
> >     Also wonder if you should add your code to a StackOverflow answer.
> 
> Alex Clemmer wrote:
>     Who do you think we should contact about this?

Marking this as "dropped" and we'll take it offline.


> On Nov. 4, 2015, 1:02 a.m., Michael Hopcroft wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line 64
> > <https://reviews.apache.org/r/39803/diff/2/?file=1113684#file1113684line64>
> >
> >     If you always return symlink.isSome(), then why use Try<>? Is it that
other users of querySymbolicLinkData() actually care about getting an error back?
> >     
> >     Even if you keep Try<> in the API, is it ok to silently ignore an error?
> 
> Alex Clemmer wrote:
>     Yep, it's a `Try` because other places we call `querySymbolicLinkData` care about
the error.
>     
>     Whether it is ok to silently ignore an error depends on what you're trying to do.
In this case, if for any reason we are unable to retrieve information about the symlink (via
the `Symlink` struct), then we report that it is not a symlink. So it is reasonable to ignore
the error, because we don't care what the error was.
>     
>     Right?

Marking this as "dropped" because there's no action item.


> 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.)
> 
> Benjamin Bannier wrote:
>     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 ;)

Marking this a "dropped" because it is used elsewhere and a fix seems outside the scope of
this review.


- Alex


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


On Jan. 5, 2016, 12:12 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39803/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2016, 12:12 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 b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe

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

>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp d46e262e0fd1c2de36f3bf19d8bd693c23bf58cd

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