[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in <new>

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added inline comments.


================
Comment at: libcxx/include/new:240
   return __align > __STDCPP_DEFAULT_NEW_ALIGNMENT__;
+#elif defined(_LIBCPP_CXX03_LANG)
+  return __align > alignment_of<__libcpp_max_align_t>::value;
----------------
ldionne wrote:
> joerg wrote:
> > ldionne wrote:
> > > So, IIUC what you're saying, `__STDCPP_DEFAULT_NEW_ALIGNMENT__` is provided by recent Clangs in C++03 mode? I tested it and it does seem to be correct. (side note: I tend to think we should be more aggressive to remove old compiler support, since most people don't upgrade their stdlib without upgrading their compiler anyway).
> > > 
> > > So if we don't care about supporting old compilers that don't provide that macro, we could just get rid of this `#elif`, and such compilers would error out when trying to use `max_align_t` in the `#else` below. That appears reasonable to me.
> > > 
> > > We'd still leave the `#if TEST_STD_VER >= 11` in the tests below, since in C++03 we wouldn't provide `std::max_align_t`, however testing that we use overaligned new in the same conditions in C++03 and C++11 becomes trivial, because it's the same code path.
> > > 
> > > Did I get what you meant correctly? If so, that sounds like a viable path forward to me, since we're simplifying the code. We're also improving on our C++03 conformance, which isn't considered good but is certainly not considered bad either.
> > Correct, it has been provided since clang 4.0 at least it seems. For testing, we have two cases, some that specifically check properties of max_align_t and those should be restricted to C++11 and newer. I think we should grow a new check that max_align_t <= __STDCPP_DEFAULT_NEW_ALIGNMENT__ as sanity check, but that's slightly OT. Most of the existing cases to check for overalignment already use __STDCPP_DEFAULT_NEW_ALIGNMENT__ anyway, so it would be a case-by-case check for those.
> I'm fine with that direction if you're willing to update the patch. I'll review it.
Do I understand it correctly that instead of `__libcpp_max_align_t` (or (over)exposed `max_align_t`) we can rely entirely on `__STDCPP_DEFAULT_NEW_ALIGNMENT__`?


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

https://reviews.llvm.org/D73245