[Headers] Remove a space in NULL define

[aheejin]

Summary

There was no space in ((void *)0) before D158709. This can cause
downstream warnings in case other libraries define NULL as
((void*)0), which is the case for musl. (see NULL definition in
https://git.musl-libc.org/cgit/musl/tree/include/stdio.h)

When a macro is redefined, if the content is the same it is fine, but if
it is different even in terms of a single space, clang warns:

../musl/include/stdio.h:37:9: error: 'NULL' macro redefined [-Werror,-Wmacro-redefined]
   37 | #define NULL ((void*)0)
      |         ^

The old code didn't have the space and it had been fine for many years,
so I think there's no risk in removing it. The linter seems to prefer
the space in there, but I think it has a risk of causing warnings or
even errors for downstream users.

[aheejin]

The added space actually caused emscripten's CI to break: https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8771230198259238865/+/u/Build_Emscripten__upstream_/stdout

[iana]

Can you add a comment that says the spacing is important please? Although why the heck is musl declaring NULL in stdio.h?

[aheejin]

Add comment

[aheejin]

Done. Btw musl defines NULL in several places, not only stdio.h. It defines NULL in local.h, stddef.h, stdio.h, stdlib.h, string.h, time.h, unistd.h, and wchar.h. Not sure why.

[aaron.ballman]

clang/lib/Headers/__stddef_null.h

18–21

My concern with this comment is that it implies we can never change our definition of NULL -- as the implementation, this is ours to define and if other C standard libraries (also part of the implementation!) are redefining the macro, IMO, they need to solve this themselves with either #ifdef or #undef as needed instead of expecting the definitions to stay in lock-step including whitespace. I don't expect us to change the definition of NULL any time soon, but this feels like a bad precedent to set. I'm not opposed (this is solving a real problem), but I'm wondering if musl would consider guarding their macro definitions so we don't have this fragility forever? We need to keep it for now so existing musl uses don't have this problem, but I don't really want to make the guarantees this comment is claiming.

[dalias]

I don't understand the motivation of trying to match musl's definition here. musl explicitly does not support using a compiler-provided stddef.h or other standard headers. If it's getting included, this is a symption of an include order problem that needs to be fixed, and getting an error telling you that is preferable.

[aaron.ballman]

In D159312#4633989, @dalias wrote:

I don't understand the motivation of trying to match musl's definition here. musl explicitly does not support using a compiler-provided stddef.h or other standard headers. If it's getting included, this is a symption of an include order problem that needs to be fixed, and getting an error telling you that is preferable.

Oh! Thank you for that information, that's really good to know! Our stddef.h is one of the few headers we provide that doesn't do an include_next to get to the system library header if one is available; I wonder if that's a contributing factor?

[aheejin]

I also think this is better solved downstream, i.e., in musl. We not a musl developer but a user, but we can consider contributing a patch there. I'd like to remove the whitespace for now because it is currently breaking us and it had been that way for a long time anyway, but I don't mind removing the comments, given that it sounds like we cannot change this forever.

[dalias]

Please report what you're actually trying to do that's breaking rather than sending patches to align definitions that are not intended to be aligned.

[aheejin]

In D159312#4634617, @dalias wrote:

Please report what you're actually trying to do that's breaking rather than sending patches to align definitions that are not intended to be aligned.

Basically, we tried to use stddef.h (from clang) and stdio.h (from musl) at the same time. Didn't know it wasn't supposed to be supported in the first place.
In more detail, our musl has some emscripten-specific modifications like this, and this lead to include stddef.h. But this problem can happen whenever we try to use the two headers together.

I get removing the whitespace here is probably not the right long-term solution. What do you think we (emscripten) or clang should do?

I also think it is a good idea to remove the comment here because we don't want to say this is a long-term solution. But I hope we keep the code as whitespace-free for now, given that at least it can be a band-aid to users like us, and it didn't have the whitespace for years before D158709 anyway.

[ributzka]

Are we keeping the comment?

[aheejin]

I uploaded D159383, which removes the comments.

[ributzka]

Thank you @aheejin

clang/lib/Headers/__stddef_null.h

 #undef NULL
 #ifdef __cplusplus
 #if !defined(__MINGW32__) && !defined(_MSC_VER)
 #define NULL __null
 #else
 #define NULL 0
 #endif
 #else
-#define NULL ((void *)0)
+// Don't add any whitespaces in ((void*)0) below!
+// musl (https://www.musl-libc.org/) redefines `NULL` as such and redefinition
+// with a different expression, even in terms of a single whitespace, causes a
+// warning.
+#define NULL ((void*)0)
 #endif