----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39584/#review110385 -----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 39) <https://reviews.apache.org/r/39584/#comment170194> What if the path ends with multiple '\' chars to begin with? 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 47) <https://reviews.apache.org/r/39584/#comment170206> What happens if the input path already contains a wildcard? Might be a good idea to check that "path" is a directory before moving forward. Also might be a good idea to use realpath here. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 51) <https://reviews.apache.org/r/39584/#comment170192> Calling FindClose on an invalid handle is not the best idea. At best it will be a no-op. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 62) <https://reviews.apache.org/r/39584/#comment170195> Nit: tou're not reusing these, so might as well inline the callse. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 73) <https://reviews.apache.org/r/39584/#comment170196> Attributes should already be available in WIN32_FIND_DATA (see https://msdn.microsoft.com/en-us/library/windows/desktop/aa365740(v=vs.85).aspx), you might not need to call GetFileAttribiutes here. If you do need to get the attributes, consider using CreateFile and GetFileInformationByHandle, which will always give the up-to-date information even on NTFS (see https://msdn.microsoft.com/en-us/library/windows/desktop/aa364952(v=vs.85).aspx for details) 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 77) <https://reviews.apache.org/r/39584/#comment170197> Consider using a smart pointer for closing the handle. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 97) <https://reviews.apache.org/r/39584/#comment170213> Failure of ::remove alone might not be reason enough for aborting the entire call. For instance, if ::remove fails with "file not found", we're still good to go. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 108) <https://reviews.apache.org/r/39584/#comment170214> Same comment as for line 108. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (lines 110 - 111) <https://reviews.apache.org/r/39584/#comment170199> Nit: error message is the same as the one on line 100, which makes it hard to tell what happened, even for someone with access to the source code. 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp (line 138) <https://reviews.apache.org/r/39584/#comment170200> Consider using ::RemoveDirectory here, which will delete the directory when the last handle is closed. Unless that's not the desired behavior... - Alex Naparu On Nov. 16, 2015, 9:13 a.m., Alex Clemmer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39584/ > ----------------------------------------------------------- > > (Updated Nov. 16, 2015, 9:13 a.m.) > > > Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph > Wu. > > > Repository: mesos > > > Description > ------- > > Windows: Implemented `os::rmdir.hpp`. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/Makefile.am > a8c35c086ecae21701f6a720f25231c1b0d4e329 > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp > 5c1df81193b4b888d2ed5c7dbfa0b5e2fae48467 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp > PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/include/stout/os/rmdir.hpp PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp > PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp > e26df59d9b837e1f4a4b92577f0a3de4b9076cb4 > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp > edf17d5ad8efbc988e909bfb8ffa5a015ecdc89d > > Diff: https://reviews.apache.org/r/39584/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 > >
