[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"

[Eric Fiselier via Phabricator via cfe-commits]

EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.

I don't think this patch is correct or desirable any longer.

I think what we should do is throw an exception right away if `__is_overaligned_for_new(align)`, and not even try to allocate. The behavior is quite surprising anyway. If new *happens* to return correctly aligned memory, then the call spuriously succeeds seemingly at random.



================
Comment at: src/experimental/memory_resource.cpp:29
+static bool is_aligned_to(void *ptr, size_t align)
+{
+    void *p2 = ptr;
----------------
Quuxplusone wrote:
> EricWF wrote:
> > 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. :)
Please change it anyway. This isn't what `std::align` was meant to do. Plus `std::align` suffers from the same problems.


================
Comment at: src/experimental/memory_resource.cpp:49
+        if (!is_aligned_to(result, align)) {
+            _VSTD::__libcpp_deallocate(result, bytes, align);
+            __throw_bad_alloc();
----------------
Oh boy... God knows if this call is correct since the value it's passing for `align` doesn't match the pointers alignment.


================
Comment at: src/experimental/memory_resource.cpp:59
+
+    bool do_is_equal(const memory_resource& other) const _NOEXCEPT override
+        { return &other == this; }
----------------
These whitespace/naming changes are unrelated.


================
Comment at: src/experimental/memory_resource.cpp:68
 {
-public:
-    ~__null_memory_resource_imp() = default;
-
-protected:
-    virtual void* do_allocate(size_t, size_t) {
-        __throw_bad_alloc();
-    }
-    virtual void do_deallocate(void *, size_t, size_t) {}
-    virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT
-    { return &__other == this; }
+    void *do_allocate(size_t, size_t) override { __throw_bad_alloc(); }
+    void do_deallocate(void *, size_t, size_t) override {}
----------------
These changes are all unrelated. 


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D47344/new/

https://reviews.llvm.org/D47344