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

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg 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:
> 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.


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

https://reviews.llvm.org/D73245