[PATCH] D157757: [Headers] Replace __need_STDDEF_H_misc with specific __need_ macros

[Ian Anderson via Phabricator via cfe-commits]

iana added inline comments.


================
Comment at: clang/lib/Headers/stddef.h:118-122
+#ifdef __cplusplus
+namespace std {
+typedef decltype(nullptr) nullptr_t;
+}
+using ::std::nullptr_t;
----------------
aaron.ballman wrote:
> ldionne wrote:
> > iana wrote:
> > > aaron.ballman wrote:
> > > > iana wrote:
> > > > > aaron.ballman wrote:
> > > > > > iana wrote:
> > > > > > > ldionne wrote:
> > > > > > > > iana wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > Related:
> > > > > > > > > > 
> > > > > > > > > > https://github.com/llvm/llvm-project/issues/37564
> > > > > > > > > > https://cplusplus.github.io/LWG/issue3484
> > > > > > > > > > 
> > > > > > > > > > CC @ldionne
> > > > > > > > > I don't _think_ this change actually changes the way nullptr_t gets defined in C++, does it?
> > > > > > > > I think we absolutely don't want to touch `std::nullptr_t` from this header. It's libc++'s responsibility to define that, and in fact we define it in `std::__1`, so this is even an ABI break (or I guess it would be a compiler error, not sure).
> > > > > > > I'm really not touching it though. All I did is move it from `__need_NULL` to `__need_nullptr_t`.
> > > > > > > 
> > > > > > > The old behavior is that `std::nullptr_t` would only be touched if (no `__need_` macros were set or if `__need_NULL` was set), and (_MSC_EXTENSIONS and _NATIVE_NULLPTR_SUPPORTED are defined).
> > > > > > > 
> > > > > > > The new behavior is that `std::nullptr_t` will only be touched if ((no `__need_` macros are set) and (_MSC_EXTENSIONS and _NATIVE_NULLPTR_SUPPORTED are defined)) or (the new `__need_nullptr_t` macro is set)
> > > > > > > 
> > > > > > > So the only change is that C++ code that previously set `__need_NULL` will no longer get `std::nullptr_t`. @efriedma felt like that was a fine change.
> > > > > > Does libc++ provide the symbols for a freestanding compilation?
> > > > > > 
> > > > > > I was pointing out those links specifically because the C++ standard currently says that stddef.h (the C standard library header) needs to provide a definition of `std::nullptr_t`, but that LWG thinks that's perhaps not the right way to do that and may be removing that requirement.
> > > > > It is weird the standard puts that in stddef.h and not cstddef. I think libc++ could provide that in their stddef.h anyway, but the intent in this review is to not rock the boat and only do the minimal change discussed above.
> > > > Yeah, this discussion is to figure out whether we have an existing bug we need to address and if so, where to address it (libc++, clang, or the C++ standard). I don't think your changes are exacerbating anything, more just that they've potentially pointed out something related.
> > > 👍 
> > > Does libc++ provide the symbols for a freestanding compilation?
> > 
> > I don't think we do. We basically don't support `-ffreestanding` right now (we support embedded and funky platforms via other mechanisms).
> > 
> > But regardless, `<stddef.h>` should never define something in namespace `std`, that should be libc++'s responsibility IMO. What we could do here instead is just
> > 
> > ```
> > #ifdef __cplusplus
> > typedef decltype(nullptr) nullptr_t;
> > #else
> > typedef typeof(nullptr) nullptr_t;
> > #endif
> > ```
> > 
> > and then let libc++'s `<cstddef>` do
> > 
> > ```
> > _LIBCPP_BEGIN_NAMESPACE_STD
> > using ::nullptr_t;
> > _LIBCPP_END_NAMESPACE_STD
> > ```
> > 
> > If Clang's `<stddef.h>` did define `::nullptr_t`, we could likely remove libc++'s `<stddef.h>` and that might simplify things.
> >> Does libc++ provide the symbols for a freestanding compilation?
> > I don't think we do. We basically don't support -ffreestanding right now (we support embedded and funky platforms via other mechanisms).
> 
> Okay, that's what I thought as well. Thanks!
> 
> > But regardless, <stddef.h> should never define something in namespace std, that should be libc++'s responsibility IMO. What we could do here instead is just
> 
> Ah, so you're thinking stddef.h should provide the global nullptr_t and cstddef should provide the std::nullptr_t. I was thinking stddef.h should not define nullptr_t in C++ mode at all; it's a C header, not a C++ header. That led me to thinking about what the behavior should be in C23 given that it supports nullptr_t.
> 
> Were it not for the current requirement that stddef.h provide nullptr_t, I think stddef.h should do:
> ```
> typedef typeof(nullptr) nullptr_t;
> ```
> in C23 mode and not do anything special for C++ at all. C's `nullptr_t` needs to be ABI compatible with C++'s `nullptr_t`, so a C++ user including the C header should not get any problems linking against a C++ library. However, this would mean that C++ users cannot include stddef.h to get nullptr_t; they'd need to include cstddef to be assured they'd get it. But because of the ABI compatibility, perhaps the solution is to expose the above in both C and C++ modes from stddef.h, then libc++ can do the dance to import it into namespace std?
Actually I think I did change it after all. If a C header does something like this
```
// It's assumed that only C23 or later is supported, or C++
#define __need_nullptr_t
#include <stddef.h>
```
If such a header got included in a C++ program, we wouldn't want to declare `std::nullptr_t`. I think we need to keep the _MSC_EXTENSIONS check in there and never declare it if that isn't set, even if the includer asked for nullptr_t. That matches the behavior of wchar_t in this header, and I think for similar reasons.

Otherwise clang's stddef.h would step on the `nullptr_t` declared by libc++'s stddef.h (modules would probably complain about a duplicate/conflicting declaration)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157757/new/

https://reviews.llvm.org/D157757