> On Oct. 26, 2015, 8:43 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp, line 
> > 42
> > <https://reviews.apache.org/r/39584/diff/2/?file=1105044#file1105044line42>
> >
> >     `X:\blah*` isn't clear.  Did you mean `currentPath` + `*`?

I actually specifically avoided mentioning the variable name because I thought 
it was not clear what the relationship of the Kleene star was to the variable. 
I've expanded the path to something more clear. What do you think of this 
solution? Is it more clear? Is it worse?


> On Oct. 26, 2015, 8:43 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp, 
> > lines 35-40
> > <https://reviews.apache.org/r/39584/diff/2/?file=1105044#file1105044line35>
> >
> >     Use `strings::endsWith`.
> >     
> >     And you might also want to use a ternary conditional:
> >     `currentPath = path + (strings::endsWith("\") ? "" : "\");`

The `strings::endsWith` suggestion is really good! I actually would like to 
avoid the choice of the ternary operator if you don't mind. Even though I used 
it in a recent review in general my personal opinion is that I tend to find it 
too opaque and not much more readble.


> On Oct. 26, 2015, 8:43 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp, line 
> > 134
> > <https://reviews.apache.org/r/39584/diff/2/?file=1105044#file1105044line134>
> >
> >     You seem to be missing an `if (recursive)` here...

Wow, major +1 awesome catch I meant to go back and add this but totally spaced 
it. Great job!


- Alex


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39584/#review104054
-----------------------------------------------------------


On Oct. 27, 2015, 8:23 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39584/
> -----------------------------------------------------------
> 
> (Updated Oct. 27, 2015, 8:23 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 
> ba2836a72ceee948cb43364e80ada9f132f33d04 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 7f70c9ea7d57634b5bfd40523ba37561ec92a09a 
>   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 
> b6afe0e76366d0bc68d37097ced83a1e14828d84 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> 3e6f2aafd0f541f512025dfa683ab4178701f7c4 
> 
> 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
> 
>

Reply via email to