mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Hopcroft" <m...@hopcroft.org>
Subject Re: Review Request 39803: Windows: Implemented stout/os/stat.hpp`.
Date Wed, 04 Nov 2015 01:02:13 GMT

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


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.


3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp (line 14)
<https://reviews.apache.org/r/39803/#comment163368>

    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 edited.
    
    I notice that the Google Style Guide recommends the #ifndef version. This may be a factor
in deciding.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp (line 22)
<https://reviews.apache.org/r/39803/#comment163369>

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



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp (line 55)
<https://reviews.apache.org/r/39803/#comment163370>

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



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp (line 61)
<https://reviews.apache.org/r/39803/#comment163371>

    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.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp (line 64)
<https://reviews.apache.org/r/39803/#comment163373>

    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.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp (line 69)
<https://reviews.apache.org/r/39803/#comment163374>

    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.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp (line 81)
<https://reviews.apache.org/r/39803/#comment163376>

    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)
<https://reviews.apache.org/r/39803/#comment163378>

    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)
<https://reviews.apache.org/r/39803/#comment163381>

    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)
<https://reviews.apache.org/r/39803/#comment163385>

    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)
<https://reviews.apache.org/r/39803/#comment163387>

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



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 75)
<https://reviews.apache.org/r/39803/#comment163386>

    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)
<https://reviews.apache.org/r/39803/#comment163388>

    Google Style Guide discourages default arguments.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 89)
<https://reviews.apache.org/r/39803/#comment163377>

    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)
<https://reviews.apache.org/r/39803/#comment163389>

    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.



3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (line 93)
<https://reviews.apache.org/r/39803/#comment163366>

    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)
<https://reviews.apache.org/r/39803/#comment163364>

    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)
<https://reviews.apache.org/r/39803/#comment163384>

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