> On Oct. 26, 2015, 1: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("\") ? "" : "\");` > > Alex Clemmer wrote: > 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.
That's fine too. I just personally like using ternary for short statements like this. (Marking as fixed.) > On Oct. 26, 2015, 1: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` + `*`? > > Alex Clemmer wrote: > 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? I think it's better. You could also say that the "X" is an arbitrary drive letter. I've seen "Z" drives before, so I'm guessing "X" drives aren't unheard of. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39584/#review104054 ----------------------------------------------------------- On Oct. 27, 2015, 1:33 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, 1:33 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 > >
