> 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))`
> 
> Kevin Klues wrote:
>     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.

I'm fine with this change being submitted because the applicable domain of 
error is so small. But, I am hoping we can agree to not define semantically 
empty macros in the future on Windows codepaths in the future. So I will add 
some historical perspective to this thread. :)

The `W*` macros you bring up that made it into `windows.hpp` all made it for 
extremely pragmatic reasons. We're not at all happy _per se_ with this outcome.

For example, some of the macros are doing things like checking for the results 
of signals that do not exist on Windows; our decision calculus, essentially, 
was: (1) it hurt readability too much to `#ifdef` out their usage in the 
codebase, (2) we were too time constrained with MesosCon approaching to 
correctly factor them out correctly, and (3) the places where we were checking 
these results had a negligible effect on Windows code paths because the signal 
would never be triggered.

If `W_EXITCODE` is similarly justified, I don't quite see it. Maybe I'm missing 
something. It looks like it's used twice on non-Windows codepaths.

I'm definitely sympathetic to the idea that we do not want to decrease 
readability in the POSIX codepaths to help Windows, a minority use case, but I 
think that dumping POSIX stuff into the Windows codepaths is a really dangerous 
habit to get into, particularly when it's not needed. This has caused me, 
personally, probably a hundred hours of debugging, all told.

Let me know if I'm missing something important here, but for now, my 
recommendation is that we strongly avoid defining things we don't have a strict 
operational justification for.


- Alex


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


On Oct. 13, 2016, 12:08 a.m., Kevin Klues wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52705/
> -----------------------------------------------------------
> 
> (Updated Oct. 13, 2016, 12:08 a.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/Makefile.am b0b08d8e0d284a88bc8daa4570540659b94dc2d0 
>   3rdparty/stout/include/stout/os/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