> On Oct. 11, 2016, 8:48 p.m., Alex Clemmer wrote: > > 3rdparty/stout/include/stout/wait.hpp, line 28 > > <https://reviews.apache.org/r/52705/diff/1/?file=1529841#file1529841line28> > > > > I've been out of the game awhile-- are we wrapping comments at 70 > > characters these days?
I typically use a 72 character wrap for comments because it looks nicer. I only go up to (or near to) 80 if the line I'm commenting is around 80 characters itself. > On Oct. 11, 2016, 8:48 p.m., Alex Clemmer wrote: > > 3rdparty/stout/include/stout/wait.hpp, line 25 > > <https://reviews.apache.org/r/52705/diff/1/?file=1529841#file1529841line25> > > > > Couple of questions here: > > > > * This seems useful for POSIX platforms, but I don't really understand > > the implications of having this enabled on Windows. Windows will never > > support the signal semantics of POSIX, and in general my feeling is that > > I'd rather not define things on platforms that don't support those > > semantics, unless there is a particularly pertinent reason to. Thoughts? > > * In the 2 or 3 places we do use `W_EXITCODE`, rather than defining it > > for Windows, we simply `#ifdef` the code out. So the precedent is to just > > not use the macro. Is there a compelling reason to change this? > > > > > > Also, tiny, tiny nit question: is it style to have 2 spaces between > > `W_EXITCODE(ret, sig)` and `((ret) << 8 | (sig))` To answer both your bullets at once... If you look in `windows.hpp` all of the other `W*` macros from the posix standard are defined in there (even though they may be meaningless on windows). This is consistent with them all being defined in `sys/wait.h` on a posix compliant system. This is why I separated the two by a simple `#ifdef` Adding the code below to be added to both windows and poix systems: ``` #ifndef W_EXITCODE #define W_EXITCODE(ret, sig) ((ret) << 8 | (sig)) #endif ``` is just to cover a corner case where some libc variants don't actually define this macro (because it's not technically posix compliant). Almost all libc variants do define it, but a bug was filed against this because (apparently) `musl` does not. Given that adding this code makes both windows and any variant of libc now define all of the `W*` macros symetrically, I think this is likely the right way to do it for now. In the future when we completely strip out all `W*` macros from windows, we can revisit this. --- Regarding the formatting question. I just copy and pasted this from the glibc header file. I don't think we have a preferred style for this, but I tried it both ways just now, and it seems to be more readable as 2 spaces, so I'll leave it. > On Oct. 11, 2016, 8:48 p.m., Alex Clemmer wrote: > > 3rdparty/stout/include/stout/wait.hpp, line 17 > > <https://reviews.apache.org/r/52705/diff/1/?file=1529841#file1529841line17> > > > > How much work is it to do this now? If we're going to have a dedicated > > header, I would greatly prefer to put all the definitions in one place. See comment below for why it probably makes sense to just leave things as they are for now - Kevin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52705/#review152196 ----------------------------------------------------------- On Oct. 10, 2016, 9:22 p.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52705/ > ----------------------------------------------------------- > > (Updated Oct. 10, 2016, 9:22 p.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-6310 > https://issues.apache.org/jira/browse/MESOS-6310 > > > Repository: mesos > > > Description > ------- > > This was motivated by the need for a default definition of > 'W_EXITCODE' (since it is not technically POSIX compliant). > > > Diffs > ----- > > 3rdparty/stout/include/stout/wait.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/52705/diff/ > > > Testing > ------- > > sudo GTEST_FILTER="*Nested*" src/mesos-tests > > > Thanks, > > Kevin Klues > >
