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

[Joerg Sonnenberger via Phabricator via llvm-commits]

joerg marked an inline comment as done.
joerg added inline comments.


================
Comment at: libcxx/include/new:229-237
+#if !defined(_LIBCPP_CXX03_LANG)
+using __libcpp_max_align_t = max_align_t;
+#else
+union __libcpp_max_align_t {
+  void * __f1;
+  long long int __f2;
+  long double __f3;
----------------
EricWF wrote:
> rsmith wrote:
> > Is there any ODR risk from this, or similar? Does libc++ support building in mixed C++98 / C++11 mode? If different TUs disagree on this alignment, we can end up allocating with the aligned allocator and deallocating with the unaligned allocator, which is not guaranteed to work.
> > 
> > We could always use the union approach if we don't know the default new alignment. But from the code below it looks like we might only ever use this if we have aligned allocation support, in which case we can just assume that the default new alignment is defined. So perhaps we can just hide the entire definition of `__is_overaligned_for_new` behind a `#ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__` and never even consider `max_align_t`?
> `max_align_t` should be ABI stable. I would rather we copy/paste the clang definition into the libc++ sources so we can use it when it's not provided by the compiler.
> 
> Though this raises another can of worms because GCC and Clang don't agree on a size for `max_align_t`.
max_align_t doesn't exist in C++03 mode, the included version is the nearest thing possible in portable C++. A visible difference happens if:
(1) The user requires C++03
(2) The code contains non-standard types with either explicit alignment or larger implicit alignment than the base types.
(3) __STDCPP_DEFAULT_NEW_ALIGNMENT__ or alignof(max_align_t) is larger than the alignment of the union.
In that case, behavior changes as to which allocator/deallocator is used. If the explicit aligned version is not compatible and life-time crosses into c++11 mode, it could be a problem. But at that point I think we did all we could possible do to provide compatibility and the code is too far in implementation-defined land already.


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

https://reviews.llvm.org/D73245