> On Jan. 14, 2016, midnight, Michael Park wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp, > > line 126 > > <https://reviews.apache.org/r/39803/diff/8/?file=1192868#file1192868line126> > > > > I noticed the use of `WindowsError` below. Should this return > > `WindowsError` as well? Here and below. > > Alex Clemmer wrote: > Probably not. `WindowsError` is similar to `ErrnoError`, but instead of > capturing the error information from `errno` (which is populated when the C > standard library encounters a runtime error), it captures the error from > `GetLastError` (which is populated when the Windows API encounters a runtime > error). At this point in this function, we will not have had the opportunity > to trigger a runtime error in the Windows API. Hence, we would not use > `WindowsError`.
I see, cool. Thanks! > On Jan. 14, 2016, midnight, Michael Park wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp, > > lines 158-162 > > <https://reviews.apache.org/r/39803/diff/8/?file=1192868#file1192868line158> > > > > ``` > > bool resolvedPathIsDirectory = > > ::_stat(absolutePath.c_str(), &s) >= 0 && S_ISDIR(s.st_mode); > > ``` > > Alex Clemmer wrote: > I actually would like to keep this how it is, if you don't mind. I think > it is less clear the other way, and it's not that much shorter. Ok, it's not a big deal. I actually meant to suggest: ```cpp const bool resolvedPathIsDirectory = ::_stat(absolutePath.c_str(), &s) >= 0 && S_ISDIR(s.st_mode); ``` Just to clarify the motivation for the suggestion, it's not for brevity. It's more so that we can't tell whether we're done initializing or not, looking at the following piece of code: ```cpp bool resolvedPathIsDirectory = false; if (::_stat(absolutePath.c_str(), &s) >= 0) { resolvedPathIsDirectory = S_ISDIR(s.st_mode); } // Hm, are there more `if` statements below where we set `resolvedPathIsDirectory` differently? ``` We could also achieve this by using a lambda, which is the recommended approach in the C++ community although not prevalent in Mesos yet. ```cpp const bool resolvedPathIsDirectory = [] { bool result = false; if (::_stat(absolutePath.c_str(), &s) >= 0) { result = S_ISDIR(s.st_mode); } return result; }(); ``` > On Jan. 14, 2016, midnight, Michael Park wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp, > > line 229 > > <https://reviews.apache.org/r/39803/diff/8/?file=1192868#file1192868line229> > > > > 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. > > Alex Clemmer wrote: > Oh I had no idea. Thanks for telling me. > > I'm obviously no expert, but just for my own understanding: this _may_ > induce undefined behavior, if `DeviceIoControl` actually modifies `ignored`, > right? But casting from const is not in itself undefined behavior? > > Either way, this is good to know. Yep, you're correct. Casting away `const` itself is legal, but modifying a `const` variable via a pointer or reference that had its `const` casted away induces undefined behavior. Presumably `DeviceIoControl` does write to it, since it's documented as an "out" parameter. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39803/#review114299 ----------------------------------------------------------- On Jan. 14, 2016, 10:08 a.m., Alex Clemmer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39803/ > ----------------------------------------------------------- > > (Updated Jan. 14, 2016, 10:08 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 > ec851dcb08d938defeb6066810011e003d594b29 > > 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 > 27edf1b4f8647a2720f2962eafa110d694b3baed > > 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 > >
