krytarowski 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; ---------------- 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__`? 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
