----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39803/#review105083 -----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 26) <https://reviews.apache.org/r/39803/#comment163470> Good comment! 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 32) <https://reviews.apache.org/r/39803/#comment163473> 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. http://stackoverflow.com/questions/10112051/c-compile-time-macros-to-detect-windows-os 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 33) <https://reviews.apache.org/r/39803/#comment163506> Consider adding one-line comments for each field. https://msdn.microsoft.com/en-us/library/cc232007.aspx 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 45) <https://reviews.apache.org/r/39803/#comment163507> One line comment for this field is important since the length is in bytes, not WCHARS. Same for line 55. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 49) <https://reviews.apache.org/r/39803/#comment163477> 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. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 51) <https://reviews.apache.org/r/39803/#comment163478> Recommend putting a blank line before this comment to that it is absolutely clear which struct it refers to. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 71) <https://reviews.apache.org/r/39803/#comment163494> 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. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 73) <https://reviews.apache.org/r/39803/#comment163475> 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. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 75) <https://reviews.apache.org/r/39803/#comment163495> 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. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 84) <https://reviews.apache.org/r/39803/#comment163481> 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. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 87) <https://reviews.apache.org/r/39803/#comment163497> 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. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 104) <https://reviews.apache.org/r/39803/#comment163501> Divide by sizeof(WCHAR). Same goes for lines 106, 111, and 113. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 108) <https://reviews.apache.org/r/39803/#comment163504> 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 its address. Same goes for line 115. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 145) <https://reviews.apache.org/r/39803/#comment163508> "or a file" not "of a file" 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 147) <https://reviews.apache.org/r/39803/#comment163509> Could you track down a more definitive source to quote? Also, perhaps you could get Microsoft to create a work item for updating MSDN. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 152) <https://reviews.apache.org/r/39803/#comment163483> The ternary operator has precedence that is sometimes confusing to some programmers. Recommend using parens to make grouping explicit. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 165) <https://reviews.apache.org/r/39803/#comment163484> Finish this. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 168) <https://reviews.apache.org/r/39803/#comment163486> Consider using some sort of RAII auto-handle that closes when it goes out of scope. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 199) <https://reviews.apache.org/r/39803/#comment163488> 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. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 201) <https://reviews.apache.org/r/39803/#comment163490> Consider swapping lines 199 and 201. Make allocation take reparsePointDataSize as a parameter. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 202) <https://reviews.apache.org/r/39803/#comment163491> Consider naming this "ignored" instead of "junk" 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 218) <https://reviews.apache.org/r/39803/#comment163493> Again, it is bad form to have the free() in two places (here and line 223). Use std::unique_ptr. 3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp (line 219) <https://reviews.apache.org/r/39803/#comment163492> Implement this. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 125) <https://reviews.apache.org/r/39803/#comment163463> 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) <https://reviews.apache.org/r/39803/#comment163464> 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) <https://reviews.apache.org/r/39803/#comment163467> 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 your function. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp (line 164) <https://reviews.apache.org/r/39803/#comment163468> 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? https://msdn.microsoft.com/en-us/library/14h5k7ff.aspx - Michael Hopcroft On Nov. 1, 2015, 1:31 a.m., Alex Clemmer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39803/ > ----------------------------------------------------------- > > (Updated Nov. 1, 2015, 1:31 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 > 67b142bbc2d80f40a1e893cc5813d58dd2aa8381 > > 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/windows/stat.hpp > 675b2e712358a55b3580026936890eaf80e5af71 > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp > 1a7037d64afeedc340258c92067e95d1d3caa027 > > 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 > >
