This is an automatically generated e-mail. To reply, visit:
I wonder if there is any good way of protecting ourselves from breaking
changes associated with _REPARSE_DATA_BUFFER. Perhaps you should consider
setting WINVER in windows.hpp. Then you could add a static_assert on the value
of WINVER in this header.
Consider adding one-line comments for each field.
One line comment for this field is important since the length is in bytes,
not WCHARS. Same for line 55.
Are you sure these buffers of size 1 are correct? I'm assuming that the
remainder of the buffer follows the _REPARSE_DATA_BUFFER in the block of memory.
I would add a comment explaining this.
Recommend putting a blank line before this comment to that it is absolutely
clear which struct it refers to.
Who actually uses this struct. The reason I ask is that it has wstrings
inside instead of strings. Will the user be forced to do the conversion?
Perhaps you should be doing the conversion for them here.
Be aware the std::wstring can throw exceptions. If stout has a strong
no-exception guarantee, you should review the code paths to make sure that this
exception is caught somewhere inside of stout.
If this struct is part of the API, it should have better documentation. One
cannot know what flags is without reading the implementation in this file.
Should check for return value of INVALID_FILE_ATTRIBUTES or add an
explanatory comment if the code on line 85 correctly handles this case.
In general, your logic should not make any assumptions on what
INVALID_FILE_ATTRIBUTES actually equals.
Oh - I see - you check for INVALID_FILE_ATTRIBUTES on line 89. In general
line 85 is bad form because reparseBitSet is only valid when attributes !=
INVALID_FILE_ATTRIBUTES. Recommend writing code so that reparseBitSet is valid
during its entire lifetime.
And get rid of this comment. It assumes that INVALID_FILE_ATTRIBUTES == -1,
which may not always be true. This comment shouldn't be necessary if you rework
the logic per my comment on line 84.
Divide by sizeof(WCHAR). Same goes for lines 106, 111, and 113.
I prefer "data.SymbolicLinkReparseBuffer.PathBuffer + printNameStartIndex",
but some might consider this a religious or stylistic difference. My opinion is
that you shouldn't dereference the pointer () if you are then going to take
Same goes for line 115.
"or a file" not "of a file"
Could you track down a more definitive source to quote?
Also, perhaps you could get Microsoft to create a work item for updating
The ternary operator has precedence that is sometimes confusing to some
programmers. Recommend using parens to make grouping explicit.
Consider using some sort of RAII auto-handle that closes when it goes out
Recommend using RAII pattern (e.g. std::unique_ptr) to eliminate leaking of
reparsePointData in the presense of exceptions or logic errors.
Since this is C++ code, recommend using new  instead of malloc/free.
Consider swapping lines 199 and 201. Make allocation take
reparsePointDataSize as a parameter.
Consider naming this "ignored" instead of "junk"
Again, it is bad form to have the free() in two places (here and line 223).
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 125)
It is probably good practice to include information about the site of the
error. First idea is to just adopt a convention of prefixing each method with
"functionName:". Better idea is to modify error.hpp so that ErrnoError()
becomes a macro that prepends the file name and line number.
This comment applies to all of your functions.
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 128)
Might want to static_assert that _USE_32BIT_TIME_T is not defined. Probably
sound advice for _UNICODE and _MBCS as well. Perhaps these static asserts
should go in windows.hpp? If you put them there, add a comment saying why the
asserts are necessary (e.g. "Implementation of mtime() assumes ...")
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 152)
Have you checked all of the return types to ensure they are what you think
they are? Windows has a lot of #ifdefs for these types and it is possible that
an unexpected cast could cause problems. I would look at the definition of each
field type in _stat and add static asserts where necessary to ensure the type
is what you think it is. Another possibility is that your functions return
types like "unsigned int" and perform the casting from s.st_dev directly in
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 164)
According to the Windows documentation, "The inode, and therefore st_ino,
has no meaning in the FAT, HPFS, or NTFS file systems." Will this be a problem?
- Michael Hopcroft
On Nov. 1, 2015, 1:31 a.m., Alex Clemmer wrote:
> This is an automatically generated e-mail. To reply, visit:
> (Updated Nov. 1, 2015, 1:31 a.m.)
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van
> Remoortere, and Joseph Wu.
> Repository: mesos
> Windows: Implemented stout/os/stat.hpp`.
> Diff: https://reviews.apache.org/r/39803/diff/
> `make check` from autotools on Ubuntu 15.
> `make check` from CMake on OS X 10.10.
> Ran `check` project in VS on Windows 10.
> Alex Clemmer