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