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