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

Reply via email to