Changeset View
Standalone View
src/experimental/memory_resource.cpp
| Show All 18 Lines | |||||
| // memory_resource | // memory_resource | ||||
| //memory_resource::~memory_resource() {} | //memory_resource::~memory_resource() {} | ||||
| // new_delete_resource() | // new_delete_resource() | ||||
| class _LIBCPP_TYPE_VIS __new_delete_memory_resource_imp | class _LIBCPP_TYPE_VIS __new_delete_memory_resource_imp | ||||
| : public memory_resource | : public memory_resource | ||||
| { | { | ||||
EricWF: Wait, can't this be written `reinterpret_cast<uintptr_t>(ptr) % align == 0`? | |||||
Yes on sane platforms, but that's technically implementation-defined behavior: the low bits of the uintptr_t may not correspond to the "alignment" of the original pointer (for example, on some hypothetical tagged-pointer architecture?). I don't know if libc++ cares about those platforms, but it seems like std::align was added specifically to solve this problem in a portable way, so I thought it would be best to use it. If you reply and say "yeah but please change it anyway," I'll change it anyway. :) Quuxplusone: Yes on sane platforms, but that's technically implementation-defined behavior: the low bits of… | |||||
Please change it anyway. This isn't what std::align was meant to do. Plus std::align suffers from the same problems. EricWF: Please change it anyway. This isn't what `std::align` was meant to do. Plus `std::align`… | |||||
| public: | void *do_allocate(size_t size, size_t align) override { | ||||
| ~__new_delete_memory_resource_imp() = default; | #ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION | ||||
Why are you removing this? EricWF: Why are you removing this? | |||||
Just removing unnecessary cruft. Same reason I added override (and removed virtual) on the other methods, and removed protected:, and removed the __ sigilling on function parameters in this .cpp file. I had done a lot more random cleanup originally, and then you said "unrelated, please revert" so I did, but I assume I just kept all the cleanups in this one class since I was touching this range of lines no matter what. Quuxplusone: Just removing unnecessary cruft. Same reason I added `override` (and removed `virtual`) on the… | |||||
I strongly prefer seeing explicit declarations of non-trivial or virtual destructors. EricWF: I strongly prefer seeing explicit declarations of non-trivial or virtual destructors. | |||||
| if (__is_overaligned_for_new(align)) | |||||
| protected: | __throw_bad_alloc(); | ||||
Also, I'm not sure about the size != 0 check. The returned pointer may be non-null and incorrectly aligned. EricWF: Also, I'm not sure about the `size != 0` check. The returned pointer may be non-null and… | |||||
This was covered in my commit message/summary: "If n == 0, return an unspecified result." We must choose here between "allocating zero bytes sometimes returns an underaligned pointer" and "allocating zero bytes sometimes throws bad_alloc." Neither one seems like good QoI. But as I no longer see why I thought "underaligned pointer" was permitted, I will change it to "sometimes throws bad_alloc" and re-upload. Quuxplusone: This was covered in my commit message/summary: "If n == 0, return an unspecified result."… | |||||
Hm! This and your other comment play into each other unfortunately well. When size == 0, the pointer we get back from __libcpp_allocate actually points to zero bytes, which means that when I call std::align on it, I invoke undefined behavior. My new theory is "I don't care about that undefined behavior." I'd still rather take undefined-behavior-in-a-rare-case over implementation-defined-behavior-in-all-cases, and I'm too lazy to implement implementation-defined-behavior-only-in-a-rare-case unless you tell me to. :P Quuxplusone: Hm! This and your other comment play into each other unfortunately well. When `size == 0`, the… | |||||
Now fixed. (The fix is always in the last place you look!) Quuxplusone: Now fixed. (The fix is always in the last place you look!) | |||||
| virtual void* do_allocate(size_t __size, size_t __align) | #endif | ||||
| { return _VSTD::__libcpp_allocate(__size, __align); /* FIXME */} | return _VSTD::__libcpp_allocate(size, align); | ||||
| } | |||||
| virtual void do_deallocate(void* __p, size_t __n, size_t __align) { | void do_deallocate(void *p, size_t n, size_t align) override { | ||||
| _VSTD::__libcpp_deallocate(__p, __n, __align); /* FIXME */ | _VSTD::__libcpp_deallocate(p, n, align); | ||||
| } | } | ||||
| virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT | bool do_is_equal(memory_resource const & other) const _NOEXCEPT override | ||||
| { return &__other == this; } | { return &other == this; } | ||||
| public: | |||||
These whitespace/naming changes are unrelated. EricWF: These whitespace/naming changes are unrelated. | |||||
| ~__new_delete_memory_resource_imp() override = default; | |||||
| }; | }; | ||||
Oh boy... God knows if this call is correct since the value it's passing for align doesn't match the pointers alignment. EricWF: Oh boy... God knows if this call is correct since the value it's passing for `align` doesn't… | |||||
| // null_memory_resource() | // null_memory_resource() | ||||
| class _LIBCPP_TYPE_VIS __null_memory_resource_imp | class _LIBCPP_TYPE_VIS __null_memory_resource_imp | ||||
| : public memory_resource | : public memory_resource | ||||
| { | { | ||||
| public: | public: | ||||
| ~__null_memory_resource_imp() = default; | ~__null_memory_resource_imp() = default; | ||||
| protected: | protected: | ||||
| virtual void* do_allocate(size_t, size_t) { | virtual void* do_allocate(size_t, size_t) { | ||||
| __throw_bad_alloc(); | __throw_bad_alloc(); | ||||
| } | } | ||||
| virtual void do_deallocate(void *, size_t, size_t) {} | virtual void do_deallocate(void *, size_t, size_t) {} | ||||
| virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT | virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT | ||||
| { return &__other == this; } | { return &__other == this; } | ||||
These changes are all unrelated. EricWF: These changes are all unrelated. | |||||
| }; | }; | ||||
| namespace { | namespace { | ||||
| union ResourceInitHelper { | union ResourceInitHelper { | ||||
| struct { | struct { | ||||
| __new_delete_memory_resource_imp new_delete_res; | __new_delete_memory_resource_imp new_delete_res; | ||||
| __null_memory_resource_imp null_res; | __null_memory_resource_imp null_res; | ||||
| ▲ Show 20 Lines • Show All 89 Lines • Show Last 20 Lines | |||||
Wait, can't this be written reinterpret_cast<uintptr_t>(ptr) % align == 0?