Differential D109056
[libc++] Disallow volatile types in std::allocator Authored by jloser on Sep 1 2021, 5:39 AM.
Details
LWG 2447 is marked as Complete, but there is no static_assert to Add static_assert in std::allocator to disallow volatile types. Since this Remove tests that use containers backed by std::allocator that test
Diff Detail
Event TimelineComment Actions Instead, would it be possible to add a static_assert in std::allocator that we're not passing a volatile type, and leave this as completed? Technically, I think this requires no change at all (and so we are correct in saying that we implement it) since the program is free to be ill-formed if one passes a volatile type to std::allocator (well, the resulting std::allocator<T> is not required to be a Cpp17Allocator). But I think it's good to explicitly error-out. We could also add a libc++ specific test for that behavior. Comment Actions Add static_assert in std::allocator to reject cv volatile types. Comment Actions That works for me. I just added a static_assert in std::allocator along with a libc++ verify test. Let me know what you think. We can gate the static_assert on C++17 and later if you prefer (since it was raised as a C++17 issue); right now I left it to apply to all standards. Comment Actions We have some tests that show we can use std::make_shared<T> when T is volatile type (see make_shared.volatile.pass.cpp). If we want this to work still, we can't have the static_assert in std::allocator. @ldionne do you still think we should reject volatile types in std::allocator? Comment Actions Update std::make_shared tests to not worry about testing support for volatile types. Comment Actions libstdc++ actually broke and then deliberately restored support for make_shared<volatile T> in GCC 8, circa September 2018. If the only problem is that make_shared<T> has an unnecessary dependency on allocator<T>, maybe we could do something like --- a/libcxx/include/__memory/shared_ptr.h +++ b/libcxx/include/__memory/shared_ptr.h @@ -1109,7 +1109,7 @@ template<class _Tp, class ..._Args, class = __enable_if_t<!is_array<_Tp>::value> _LIBCPP_HIDE_FROM_ABI shared_ptr<_Tp> make_shared(_Args&& ...__args) { - return _VSTD::allocate_shared<_Tp>(allocator<_Tp>(), _VSTD::forward<_Args>(__args)...); + return _VSTD::allocate_shared<_Tp>(allocator<int>(), _VSTD::forward<_Args>(__args)...); } to work around that. The allocator gets rebound anyway, so there's no real reason to bother instantiating allocator<_Tp> in particular... and all allocator<T>s happen to have the same layout, so it wouldn't even break ABI to do this IMHO. FWIW I also think the status quo is fine: so allocator<volatile T> is IFNDR instead of a static_assert; does anyone really care? Comment Actions Simply put, I disagree with Jonathan's call on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87278. As he said there, the Committee is moving away from supporting volatile in the standard library, and I think that sort of benign breakage is acceptable. After all, the Standard says that allocator<volatile T> is IFNDR, so we shouldn't encourage people to use that type.
Comment Actions Remove tests that use containers with a (const) volatile type since they would trigger the static_assert. Comment Actions LGTM once CI passes.
Comment Actions Do we want to land this if CI is passing shortly or do internal builds at Apple/Google first for this breakage? Comment Actions Let's land this, I don't foresee any issues, and we can always revert in case something bad happens. I'll run a build in the meantime. Please apply my suggested change just for consistency.
| ||||||||||||||||||||||||