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

This is my initial set of comments. I still need to read from line 112 on in 
stat.hpp and I haven't started reading reparsepoint.hpp.

(line 14)

    Recommend #pragma once. This is supported by VS. GCC supports it since 
version 3.4 (https://en.wikipedia.org/wiki/Pragma_once), but this shouldn't 
matter since we're discussing a Windows file. In general #pragma once is 
preferred over the #ifndef include guard mechanism, so the main question would 
pertain to consistency with the rest of the codebase. My recommendation is to 
use #pragma once for all new code and gradually migrate old code as it is 
    I notice that the Google Style Guide recommends the #ifndef version. This 
may be a factor in deciding.

(line 22)

    Not sure how lines 22 and 24 fit with the #include file ordering scheme.

(line 55)

    Google Style Guide allows auto if it help readability? Is it a good or bad 
idea here?

(line 61)

    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.

(line 64)

    Google Style Guide isn't really clear on this one. I would recommend 
indenting line 66 to align with the open paren on line 65.

(line 69)

    I'd put a shorter comment here, something like "Open `HANDLE` for the 
symlink isself." In general I feel it is error prone to include a verbatem copy 
of the function's documentation here as it won't get updated, but could be 
confused for the function's documentation.

(line 81)

    Is it possible to leak a handle in the presence of an error? I guess if 
getSymbolicLinkData() can never throw you will always execute line 81.
    Still, I would rather see RAII pattern involving any code with handles, 
just to be safe. This also future proofs the code against edits that make the 
logic more complex.

3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 17)

    A quick read doesn't turn up any use of <cstring>. Since I don't really 
know the stout API all that well, I would recommend attempting a compile with 
each new header removed to generate proof that they are being used. Also keep 
in mind that they must be required by stat.hpp, and not some other header than 
includes it.

3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 39)

    Might add a comment explaining that when ::stat() returns a value less than 
zero the error can only be ENOENT or EINVAL. ENOENT means the path doesn't 
exist, so returning false is correct. EINVAL should be impossible, based on 
inspection of the code.

3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 62)

    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?

3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 68)

    Is FollowSymLink your enum or an enum that already exists in the Linux 

3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 75)

    When you say the size of the file system entry, I assume you mean the size 
of the file pointed to, not the number of bytes in the file system record. 
Might want to reword the comment to make it more clear.
    Also, what happens if the path is a directory?

3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 81)

    Google Style Guide discourages default arguments.

3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 89)

    I'll just ask this once here - is string concatendation the accepted way of 
building up error messages in Stout?

3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 105)

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

3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (line 93)

    Recommend specifying in the comment that this is a Windows stat structure 
(from <stat.h>).

3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (line 94)

    Why make these macros instead of functions?
    Also, shouldn't these be in a header under internal? Why does anyone else 
outside of stout need to see them?

3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (line 95)

    Are you sure this is the correct meaning of S_IFREG? Is it possible for 
both S_IFDIR and S_IFREG to be set? The windows documentation states, "the 
_S_IFREG bit is set if path specifies an ordinary file or a device." What 
happens if the path is "c:\". This is a device and also a directory.

- Michael Hopcroft

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

Reply via email to