aaron.ballman 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;
----------------
ldionne wrote:
> iana wrote:
> > 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)
> @aaron.ballman Ok, I follow your train of thought. So in that case what we
> would do is this:
>
> ```
> // <stddef.h> from the Clang builtin headers:
> #if we-are-in-C23
> typedef typeof(nullptr) nullptr_t;
> #endif
> // never do anything special for C++
> ```
>
> ```
> // <cstddef> from libc++:
> #include <stddef.h>
>
> #if we-are-in-whatever-C++-standard-synced-with-C23
> _LIBCPP_BEGIN_NAMESPACE_STD
> using ::nullptr_t; // use the C23 ::nullptr_t as defined in <stddef.h>
> _LIBCPP_END_NAMESPACE_STD
> #else
> _LIBCPP_BEGIN_NAMESPACE_STD
> typedef decltype(nullptr) nullptr_t; // declare our own nullptr_t from
> scratch since C doesn't have a notion of it
> _LIBCPP_END_NAMESPACE_STD
> #endif
> ```
>
> This way:
> 1. Libc++ doesn't need to have `<stddef.h>` anymore
> 2. The Clang builtin headers are pure C
> 3. `std::nullptr_t` and `::nullptr_t` are always the same thing when both are
> defined
>
> However in this world `::nullptr_t` would not be defined when including
> `<stddef.h>` in C++ prior to whatever version is synced with C23 (that's not
> a problem IMO, but it is a behavior change).
>
> Does this make sense to everyone?
> Does this make sense to everyone?
I think it makes sense, yes (thank you for spelling it out so clearly!). I
agree that it's a behavior change, but it's one I'm hoping we can argue for in
LWG when discussing https://cplusplus.github.io/LWG/issue3484 -- but that said,
I'm not certain if this behavior change will cause problems for significant
third-party library or system headers.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157757/new/
https://reviews.llvm.org/D157757
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
