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

[Louis Dionne via Phabricator via llvm-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
Herald added a subscriber: dexonsmith.


================
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;
----------------
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.


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

https://reviews.llvm.org/D73245