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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
