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

Reply via email to