> On May 10, 2016, 5:41 p.m., Alex Clemmer wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp, lines 397-398 > > <https://reviews.apache.org/r/47168/diff/1/?file=1377774#file1377774line397> > > > > I think these are semantically identical, so we might even just add > > theese as constants above, in the same way we do for other stuff like > > `S_IRUSR`.
Not sure what is the comment. > On May 10, 2016, 5:41 p.m., Alex Clemmer wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp, line > > 52 > > <https://reviews.apache.org/r/47168/diff/1/?file=1377773#file1377773line52> > > > > Because `pid_t` is signed on POSIX and unsigned on Windows, we need to > > very purposeful about the behavior here. You can see our approach to > > `waitpid` in a previous review: https://reviews.apache.org/r/46340/ > > > > Let me say two things in refernce to this issue. > > > > First, in this case I think it is arguable that we are being overly > > cautious with the `pid_t` handling here, because some CRT APIs do use `int` > > to represent `pid`s (for example `_getpid` does this). In this case, extra > > caution costs is basically nothing (just the cost of writing a couple > > comments) and the result that we get out of it is (1) no subtle bugs, and > > (2) a junior engineer can maintain the code and understand fully the > > assumptions that would otherwise be implicit in the code. I personally > > would recommend a similar approach to this in `kill`. > > > > Second, my belief is that the "weird" numbers like `-1` are not used > > pervasively in the Mesos codebase, so I see nothing particularly wrong with > > limiting the set of arguments we are using in the same way we did with > > `os::waitpid`. > > > > Curious to hear your thoughts. Will talk offline. - Daniel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47168/#review132482 ----------------------------------------------------------- On May 10, 2016, 8:54 a.m., Daniel Pravat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47168/ > ----------------------------------------------------------- > > (Updated May 10, 2016, 8:54 a.m.) > > > Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van > Remoortere, and Michael Park. > > > Repository: mesos > > > Description > ------- > > Windows: Implement `kill` and `killtree`. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/Makefile.am > 33ddb06e25920096f2d16d4f372129ee2f6a8721 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/kill.hpp PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/kill.hpp > PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp > PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp > 3828c53c0dbbf370d642189998af75b9af434e9d > > Diff: https://reviews.apache.org/r/47168/diff/ > > > Testing > ------- > > Windows: build/test w/ Marathon > > > Thanks, > > Daniel Pravat > >
