[libcxx-commits] [PATCH] D114786: [libc++] Remove the ability to use the std::nullptr_t emulation in C++03 mode

Mon Mar 7 05:32:56 PST 2022

ldionne marked 4 inline comments as done.
ldionne added inline comments.

================
Comment at: libcxx/include/stddef.h:48
 #ifdef __cplusplus
-
-extern "C++" {
-#include <__nullptr>
-using std::nullptr_t;
-}
-
+    typedef decltype(nullptr) nullptr_t;
 #endif
----------------
JamesNagurne wrote:
> Quuxplusone wrote:
> > JamesNagurne wrote:
> > > ldionne wrote:
> > > > Quuxplusone wrote:
> > > > > @JamesNagurne @ldionne : Whoa, yeah. Shouldn't this be
> > > > > ```
> > > > > #ifdef __cplusplus
> > > > > namespace std { using nullptr_t = decltype(nullptr); }
> > > > > #endif
> > > > > ```
> > > > > and that's all? Also, what was this ever doing in C's <stddef.h>? It should go in <cstddef> instead.
> > > > > If we want to preserve libc++'s historical behavior, it seems it would be
> > > > > ```
> > > > > // in <cstddef>
> > > > > namespace std { using nullptr_t = decltype(nullptr); }
> > > > > 
> > > > > // in <stddef.h>
> > > > > namespace std { using nullptr_t = decltype(nullptr); }
> > > > > using std::nullptr_t;
> > > > > ```
> > > > > (Yes, including the same `using nullptr_t = decltype(nullptr);` token-sequence twice in a row is actually fine with C++ these days. I checked that at least GCC trunk's C++11 mode and MSVC latest's C++11 mode are happy with it.)
> > > > > But, if we want to do the most sensible thing regardless of backward compatibility, I think it would be
> > > > > ```
> > > > > // in <cstddef>
> > > > > namespace std { using nullptr_t = decltype(nullptr); }
> > > > > 
> > > > > // in <stddef.h>
> > > > > /* nothing at all */
> > > > > ```
> > > > > 
> > > > > I propose to leave this to @ldionne to address, unless he delegates it back to me. :)
> > > > @JamesNagurne @Quuxplusone 
> > > > 
> > > > I agree it's messed up that we are defining `::nullptr_t`, however that was the state of things before this patch too, wasn't it? I don't understand why https://godbolt.org/z/GjbEh1hqT shows different behavior, I must be missing something. Certainly this patch did not intend to change whether we provided `::nullptr_t` or not.
> > > > 
> > > > Edit: OH, now I see it. We've always been defining `::nullptr_t` from `<stddef.h>`, but now we also define it from `<cstddef>`, and that's the behavior change. That was not intended.
> > > > 
> > > > @Quuxplusone are we really allowed to not define `::nullptr_t` at all from `<stddef.h>`?
> > > Not Arthur, but:
> > > 
> > > C++14 18.2 describes <cstddef>
> > > - 18.2.2: Contents are the same as stddef.h (I suspect this then implies the requirements of C11's stddef.h description), except:
> > > - 18.2.9: nullptr_t is a namespace std typedef to decltype(nullptr)
> > > 
> > > C11 Annex B.18
> > > - Unsurprisingly, no reference to nullptr_t
> > > 
> > > C++14 Annex D.5.2, going to quote it to avoid ambiguity:
> > > > Every C header, each of which has a name of the form name.h, behaves as if each name placed in the standard library namespace by the corresponding cname header is placed within the global namespace scope. It is unspecified whether these names are first declared or defined within namespace scope (3.3.6) of the namespace std and are then injected into the global namespace scope by explicit using-declarations (7.3.3).
> > > 
> > >   - std::nullptr_t defined in cstddef
> > >   - stddef.h places that name into global namespace scope either through injection of std::nullptr_t or through a compatible redefinition
> > > 
> > > This allows for nullptr_t to be in the global namespace when including stddef.h. However, the standard is pretty clear that <cstddef> shall not define a global nullptr_t name.
> > > 
> > > 
> > > 
> > > 
> > > 
> > @ldionne: Oops! I //assumed// that `<stddef.h>` was supposed to act like C's `/usr/include/stddef.h` and thus couldn't possibly be expected to define C++ `nullptr_t`; but @JamesNagurne has quoted chapter and verse that proves my assumption was wrong.
> > https://eel.is/c++draft/support.c.headers#other-1 (stddef.h must include all of cstddef's std stuff, but into the global namespace)
> > https://eel.is/c++draft/cstddef.syn#lib:nullptr_t (cstddef includes std::nullptr_t)
> > So yup, <stddef.h> //must// include `nullptr_t` in the global namespace.
> > 
> > > However, the standard is pretty clear that <cstddef> shall not define a global nullptr_t name.
> > 
> > I think we're all on the same page that //as a matter of QoI, we don't **want** <cstddef> to define a global ::nullptr_t.// But FWIW I think the standard is very clear that <cstddef> //could// do that if it wanted to:
> > https://eel.is/c++draft/support.c.headers#other-2.sentence-2 "It may also provide these names within the global namespace." (This is a non-normative example, though, and I'm not sure there's any normative text that explains the example's reasoning any more fully.)
> Consider me 'lawyered'!
> 
> My opinion, FWIW, would be to maintain consistency with the major C++ libs (Microsoft, libstdc++), if there is consistency. But I do defer to y'all.
What @Quuxplusone quoted is what I had read.

Consistency with other implementations seems to be that `<cstddef>` **does** define `::nullptr_t`: https://godbolt.org/z/1rnv5v43G. Given this, I think it makes sense to not jump through hoops to try to be inconsistent with other implementations -- after all, portability is a quality, not a defect. So I would be tempted to not change our status quo, except I'll add a libc++ specific test to check that we do define `::nullptr_t` when including `<cstddef>`, since that was indeed an unintended behavior change of this patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114786