> 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
> 
>

Reply via email to