[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 marked 5 inline comments as done.
joerg added inline comments.


================
Comment at: libcxx/include/stddef.h:58
     !defined(__DEFINED_max_align_t) && !defined(__NetBSD__)
 typedef long double max_align_t;
 #endif
----------------
ldionne wrote:
> Why do we need this at all anymore? In C++11, can't we rely on the C Standard Library providing it `::max_align_t`?
Sorry, left-over. Will be killed.


================
Comment at: libcxx/test/libcxx/experimental/memory/memory.resource.adaptor/memory.resource.adaptor.mem/db_deallocate.pass.cpp:40
+#ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__
+    std::size_t maxSize = std::numeric_limits<std::size_t>::max()
+                            - __STDCPP_DEFAULT_NEW_ALIGNMENT__;
----------------
ldionne wrote:
> This test is only enabled in >= C++11, so I think `std::max_align_t` should always be provided. So why do we want that `#if`?
We know that the new alignment can be stricter than max_align_t and we want to test the overflow case here. So going for the strictest alignment is more sensible.


================
Comment at: libcxx/test/std/language.support/support.types/max_align_t.pass.cpp:47
+#ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__
+    static_assert(std::alignment_of<std::max_align_t>::value <=
+                  __STDCPP_DEFAULT_NEW_ALIGNMENT__,
----------------
ldionne wrote:
> Since we now assume that `__STDCPP_DEFAULT_NEW_ALIGNMENT__` is defined, can we remove that `#if` and keep the assertion?
It's only required in C++03 mode. We still support C++11 and higher without it, e.g. for gcc.


================
Comment at: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp:275
+#if TEST_STD_VER >= 11
+    const size_t alignment = TEST_ALIGNOF(std::max_align_t) > 16 ?
+        16 : TEST_ALIGNOF(std::max_align_t);
----------------
ldionne wrote:
> Can you walk me through why the check for `> 16` is required?
If max_align_t is 256bit, we still only expect 128bit alignment (long double). This test is still checking more than it should, e.g. in principle a target could legitimately have no natural type larger than 64bit, but support 256bit vector types and set max_align_t accordingly. The condition is here because std::min in C++11 is not constexpr.


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

https://reviews.llvm.org/D73245