Sorry, I'm confused, why does vim not need to do this?

edit: I understand now, the key difference is epoll.

macos gcc failure is weird:

/Users/travis/build/neovim/neovim/src/nvim/os/signal.c: In function 'signal_init':
/Users/travis/build/neovim/neovim/src/nvim/os/signal.c:51:3: error: conversion to 'sigset_t' from 'int' may change the sign of the result [-Werror=sign-conversion]
   sigaddset(&mask, SIGCHLD);
   ^

LGTM

The macos gcc error seems insane.

I think I see where the OS X error is coming from:

from signal.h:

int  sigaddset(sigset_t *, int);
#define     sigaddset(set, signo)   (*(set) |= __sigbits(signo), 0)

from sys/_types/_sigset.h:

typedef __darwin_sigset_t            sigset_t;

and from sys/_types.h:

typedef __uint32_t   __darwin_sigset_t;      /* [???] signal set */

So it seems, OS X said sigset_t is unsigned, but then ORs in an int, mixing the types. So GCC seems correct here... sadly.

Something weird is definitely going on. OSX clang (the default compiler) with -Weverything doesn't warn for:

#include <stdio.h>
#include <signal.h>

int main() {
  sigset_t s;
  sigemptyset(&s);
  sigaddset(&s, SIGCHLD);
  return 0;
}

But gcc (6) does.

-std=c11 -Wall -Wextra -Wsign-conversion sig.c -o sig.o |& sed 's/^/gcc-6: /' ; clang -std=c11 -Weverything -Wsign-conversion sig.c -o sig.o |& sed 's/^/clang: /'
gcc-6: sig.c: In function 'main':
gcc-6: sig.c:7:3: warning: conversion to 'sigset_t {aka unsigned int}' from 'int' may change the sign of the result [-Wsign-conversion]
gcc-6:    sigaddset(&s, SIGCHLD);
gcc-6:    ^~~~~~~~~

Perhaps it's using different include paths. I think I've seen something similar to this before, from way back when, when I was working on the automatic C header to Lua FFI cleaner.

Something is suppressing those warnings. Running the preprocessor with both gcc and clang generates near identical files, barring some whitespace, standard defines and macros.

Still, the code looked like it should warn, so I did:

$ clang -Weverything sig.c -o sig
<no warning>
$ clang -Weverything $(mv =(clang -E sig.c) tmp.c ; echo tmp.c) -o sig.clang
sig.c:6:13: warning: implicit conversion changes signedness: 'int' to 'unsigned int' [-Wsign-conversion]
  (*(&s) |= __sigbits(20), 0);
         ~~ ^~~~~~~~~~~~~
1 warning generated.

Hah! I got you, clang, you conniving compiler...

So the system header is broken. Is the #pragma approach in this PR the appropriate way to address that?

So the system header is broken. Is the #pragma approach in this PR the appropriate way to address that?

I kinda want to know how Apple is avoiding those warnings. But I can't find it...

I kinda want to know how Apple is avoiding those warnings. But I can't find it...
Hide all checks

So, I really can't find any pragmas. But I encountered this in the GCC docs:

-Wsystem-headers
Issue warnings for code in system headers. These are normally unhelpful in finding bugs in your own code, therefore suppressed. If you are responsible for the system library, you may want to see them.
https://gcc.gnu.org/onlinedocs/gcc/Preprocessor-Options.html

And

The header files declaring interfaces to the operating system and runtime libraries often cannot be written in strictly conforming C. Therefore, GCC gives code found in system headers special treatment. All warnings, other than those generated by ‘#warning’ (see Diagnostics), are suppressed while GCC is processing a system header. Macros defined in a system header are immune to a few warnings wherever they are expanded. This immunity is granted on an ad-hoc basis, when we find that a warning generates lots of false positives because of code in macros defined in system headers.
https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html

Which implies that system headers are usually automatically warning-free. But the case is different for macro's. Macro's in system headers are not exempt from all warnings. Perhaps -Wsign-conversion is system-macro-exempt for Clang but not GCC? I'm unsure where to find an exhaustive list of these things...

That would explain why this happens:

$ clang-3.8 -std=c11 -Weverything -Wsign-conversion sig.c -o sig.o
$ clang-3.8 -std=c11 -Weverything -Wsign-conversion $(clang-3.8 -E sig.c -o tmp.c && echo tmp.c) -o sig.o
sig.c:6:13: warning: implicit conversion changes signedness: 'int' to 'unsigned int' [-Wsign-conversion]
  (*(&s) |= __sigbits(20), 0);
         ~~ ^~~~~~~~~~~~~
1 warning generated.

...in the second case, the code is inlined, and no longer in a system header.

...and another reference from the Clang devs from 2012. Still no word on where to find exactly which warnings/macro's are considered: http://clang-developers.42468.n3.nabble.com/Warnings-from-macro-expansions-vs-system-headers-td4028109.html

Man, that pragma solution is unbelievably ugly, but it's starting to look like the only way. libuv doesn't have the same warning levels as us, AFAIK. Perhaps we should file a bug to the GCC devs to make an exception for this one. Or... a rdar:// bug for the Apple folks, but something tells me they'll be less than responsive.

Given that we know how this is generated under OS X, we could maybe just cast the argument under OS X:

sigaddset(&s, (sigset_t) SIGCHLD);

It's not great either, but less ugly than the pragma stuff and a decent comment would help future readers know why we're doing.

Just a thought.

@aktau I've filed several development related bugs in the past for similar, and they're very unresponsive to them. :-(

@jszakmeister see the first smurf commit. That did not seem to work.

@jszakmeister as @justinmk mentioned, you cannot get around it with a cast. The problem is in the implementation of sigaddset itself. Clang tells us:

sig.c:9:13: warning: implicit conversion changes signedness: 'int' to 'unsigned int' [-Wsign-conversion]
  (*(&s) |= __sigbits((sigset_t) 20), 0);
         ~~ ^~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

Thre return value of __sigbits is int, not sigset_t.

NOTE: SIGCHLD = 20.

I tried it with gcc-5 before making the suggestion, but it looks like I just missed the warning. Sorry for the noise. I also didn't see the __sigbits() in there to help screw up that trick. Oh well.

Thre return value of __sigbits is int, not sigset_t.

Right... I wasn't trying to say it was, I just missed __sigbits being in there. It is, however, trying to OR an int and an unsigned int, and it was that which I was hoping to avoid. :-)

You could go the other way... since it's the mask that it's being OR'd into, you could cast it instead. That seems to get rid of the warning:

sigaddset((int *) &mask, SIGCHLD);

Ok, we still need to detect when this should be done. Because for systems that don't have such headers we could end up with the reverse problem....

You could go the other way... since it's the mask that it's being OR'd into, you could cast it instead. That seems to get rid of the warning:

Very clever!

Ok, we still need to detect when this should be done. Because for systems that don't have such headers we could end up with the reverse problem....

Exactly.

sigaddset((int *) &mask, SIGCHLD);

I would guess that will break if Apple fixes their headers. So I think disabling the warning for that single line is the least costly approach.

Updated, including notes from this discussion in the commit message. Will merge after build.

I would guess that will break if Apple fixes their headers. So I think disabling the warning for that single line is the least costly approach.

Indeed that would be the idea. At that point we'll know they fixed their headers and we can adjust the #ifdef to only apply to old Apple platforms, up until the point we decide not to support macOS with broken headers anymore.

Said another way. if we go with the current way (pragmas), we might never know when Apple fixes this, and we might get stuck with technical debt forever. I believe this is how Vim reached its current state as well. Eternal manual vigilance vs automatic machine vigilance and a bit of fixer-upping. I know which one I'd chose.