> 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.
> 
> Alex Clemmer wrote:
>     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.

I agree on all fronts. We should always be mindful of this.


- Kevin


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