> On Jan. 14, 2016, midnight, Michael Park wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp, > > line 67 > > <https://reviews.apache.org/r/39803/diff/8/?file=1192868#file1192868line67> > > > > This looks like it's > 80 chars long. Is there maybe a shorter linik? > > If not, let's just add `// NOLINT(whitespace/line_length)`.
Running `mesos-style.py` manually over this file does not trigger the linter. Do you still want me to change it? > On Jan. 14, 2016, midnight, Michael Park wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp, > > lines 95-96 > > <https://reviews.apache.org/r/39803/diff/8/?file=1192868#file1192868line95> > > > > 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? It sounds like there might be two questions here: (1) why are we dividing by `sizeof(WCHAR)` to calculate the index of the beginning/ending offsets, and (2) why does the `SymbolicLink` data structure use `wchar`. Let me address both of them. First, the division by `sizeof(WCHAR)` is because the offsets in the `REPARSE_DATA_BUFFER` are actually _byte_ offsets, not array index offsets. Thus, to calculate the actual array index of the beginning and ending of the `printName` and `substituteName`, we need to divide by `sizeof(WCHAR)`. This converts the byte offset into an array index offset. Second, the `SymbolicLink` data structure uses `wstring` because (1) the goal is for `SymbolicLink` to be a convenient, internal-only way to interact with the data in a `REPARSE_DATA_BUFFER`, and (2) converting to `string` is really hard and not necessary for this goal. I think part of the confusion is that it seemed like the division by `sizeof(WCHAR)` might mean that these fields aren't really `wstring`, which is not the case. Does this clarify things? > 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. 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`. > 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); > > ``` 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. > 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. 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. > On Jan. 14, 2016, midnight, Michael Park wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line > > 100 > > <https://reviews.apache.org/r/39803/diff/8/?file=1192871#file1192871line100> > > > > Is this supposed to be `ErrnoError`? It looks like in the case above, > > `ErrnoError` was changed to `Error`. Do we not want `WindowsError`? It is supposed to be `ErrnoError`, because `errno` is populated during calls to the C standard library. `WindowsError` is similar, but for the Windows API. > On Jan. 14, 2016, midnight, Michael Park wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp, line > > 108 > > <https://reviews.apache.org/r/39803/diff/8/?file=1192871#file1192871line108> > > > > Is this `UNREACHABLE` necessary? It is not necessary for modern compilers, which should catch missing cases if we're not using `default` (which we aren't). Still, it is convention elsewhere in the code, and I don't see any particular reason why this isn't just good to have, so if you don't mind, I'd like to keep it. - Alex ----------------------------------------------------------- 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 > >
