ldionne 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; ---------------- krytarowski wrote: > 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__`? Yes, that is the plan IIUC. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73245/new/ https://reviews.llvm.org/D73245 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
