----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39803/#review114299 -----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 29) <https://reviews.apache.org/r/39803/#comment175122> `s/(e.g.)//` 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 67) <https://reviews.apache.org/r/39803/#comment175124> This looks like it's > 80 chars long. Is there maybe a shorter linik? If not, let's just add `// NOLINT(whitespace/line_length)`. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (lines 95 - 96) <https://reviews.apache.org/r/39803/#comment175125> I don't quite understand why these are `std::wstring`. It seems like we do some `x / sizeof(WCHAR)` to make this work? Could you elaborate on this a little bit please? 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 104) <https://reviews.apache.org/r/39803/#comment175112> This looks like it should return a `Try<bool>`? 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 112) <https://reviews.apache.org/r/39803/#comment175113> I think we can remove this variable. The intent is clear from the function name. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 121) <https://reviews.apache.org/r/39803/#comment175116> Why do we take a copy of the `std::shared_ptr` here? 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 126) <https://reviews.apache.org/r/39803/#comment175138> I noticed the use of `WindowsError` below. Should this return `WindowsError` as well? Here and below. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (lines 158 - 162) <https://reviews.apache.org/r/39803/#comment175117> ``` bool resolvedPathIsDirectory = ::_stat(absolutePath.c_str(), &s) >= 0 && S_ISDIR(s.st_mode); ``` 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 200) <https://reviews.apache.org/r/39803/#comment175158> Similar to above, why does this function need to take a copy of the `std::shared_ptr`? It looks to me like this should simply take a `void*`, and we can actually return a `std::unique_ptr<void>` from `getHandleNoFollow` instead. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (lines 227 - 228) <https://reviews.apache.org/r/39803/#comment175148> Why do we store this as a `std::shared_ptr`? Can we just store the data on the stack, and pass a pointer to it to `buildSymbolicLink`? 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 229) <https://reviews.apache.org/r/39803/#comment175151> It seems like we pass the address of this variable as a dummy out parameter below. Presumably this can't be declared `const`. It looks like would be that C-style cast of `(LPDWORD)&ignored` ignores the `const` and makes it so. This induces undefined behavior. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (lines 248 - 249) <https://reviews.apache.org/r/39803/#comment175147> `return buildSymbolicLink(reparsePointData);` 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp (line 45) <https://reviews.apache.org/r/39803/#comment175153> `s/filesysem/filesystem/` 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp (line 55) <https://reviews.apache.org/r/39803/#comment175152> Remove newline. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/symlink.hpp (lines 76 - 78) <https://reviews.apache.org/r/39803/#comment175157> ``` return getSymbolicLinkData(symlinkHandle.get()); ``` 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 98) <https://reviews.apache.org/r/39803/#comment175162> Is this supposed to be `ErrnoError`? It looks like in the case above, `ErrnoError` was changed to `Error`. Do we not want `WindowsError`? 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 106) <https://reviews.apache.org/r/39803/#comment175163> Is this `UNREACHABLE` necessary? - Michael Park On Jan. 12, 2016, 2:03 a.m., Alex Clemmer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39803/ > ----------------------------------------------------------- > > (Updated Jan. 12, 2016, 2:03 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/posix/stat.hpp > ffe074830ef90f492990bf695e686c885b9a155c > 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 > >
