iana marked an inline comment as done.
iana added inline comments.
================
Comment at: clang/lib/Headers/__stddef_null.h:14
+ * __need_NULL and rely on stddef.h to redefine NULL to the correct value
again.
+ * Modules don't support redefining macros like that, but support that pattern
+ * in the non-modules case.
----------------
dexonsmith wrote:
> iana wrote:
> > I couldn't come up with anything clever to support this in modules. If I
> > get rid of the guard, `NULL` still doesn't get redefined in modules. If I
> > move the `undef` to stddef.h then `NULL` gets undefined, but not redefined.
> > So I think precompiled modules must inherently have `#pragma once`
> > semantics. If we really had to, we could move this back into stddef.h and
> > let it be textual, but it's distasteful that `NULL` gets defined in every
> > single module that includes stddef.h and has to be merged later.
> Modules are, in a way, stronger than `#pragma once`. Each module is a
> separate translation unit. If there's a module around `__stddef_null.h`, then
> it will only be compiled once, and any definitions imported from there. Other
> importing TUs get those definitions per the context the module was compiled
> in (they don't re-compile the code in their own context...).
>
> It seems like a regression for `NULL` to degrade from `((void *)0)` to `0`
> since it loses type safety, which is rather rare and precious in C code.
> Maybe this header should remain textual when deployed to platforms that rely
> on this dance.
Sure, but I was a little surprised that this doesn't work.
```
@import _Builtin_stddef.null;
undef NULL
@import _Builtin_stddef.null;
// NULL is undefined
```
It makes perfect sense that the module doesn't get re-compiled on the second
import (and maybe not even the first), and that when it does get precompiled it
doesn't see anything that the importing code did with `NULL`. But I //am// a
bit surprised that re-importing the module doesn't re-expose everything from it.
I guess if we really need to, we could `unifdef` the module map in case some
platforms need this to be textual, and then lose the guard.
================
Comment at: clang/lib/Headers/module.modulemap:156
-module _Builtin_stddef_max_align_t [system] [extern_c] {
- header "__stddef_max_align_t.h"
----------------
benlangmuir wrote:
> Is the assumption here that directly `@import _Builtin_stddef_max_align_t` is
> unsupported so we don't need to provide a shim to keep this working?
Yes that is the assumption, but if someone knows different please let me know!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D159064/new/
https://reviews.llvm.org/D159064
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
