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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.
Herald added a project: All.


================
Comment at: libcxx/include/cstddef:43
 
-// Don't include our own <stddef.h>; we don't want to declare ::nullptr_t.
-#include_next <stddef.h>
----------------
This is where I messed up in this patch, basically. By switching from `include_next <stddef.h>` to `include <stddef.h>`, I made it so that `::nullptr_t` would be defined even when including `<cstddef>`. That being said, I really really dislike "clever" headers like this -- IMO all the `<cfoo>` headers should just include `<foo.h>` and put their declarations inside namespace `std`.


================
Comment at: libcxx/include/stddef.h:48
 #ifdef __cplusplus
-
-extern "C++" {
-#include <__nullptr>
-using std::nullptr_t;
-}
-
+    typedef decltype(nullptr) nullptr_t;
 #endif
----------------
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>`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114786