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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: src/experimental/memory_resource.cpp:29
+static bool is_aligned_to(void *ptr, size_t align)
+{
+    void *p2 = ptr;
----------------
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. :)


================
Comment at: src/experimental/memory_resource.cpp:48
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+        if (__size != 0 && !is_aligned_to(result, __align)) {
+            _VSTD::__libcpp_deallocate(result, __align);
----------------
EricWF wrote:
> Also, I'm not sure about the `size != 0` check. The returned pointer may be non-null and incorrectly aligned.
This was covered in my commit message/summary: "If n == 0, return an unspecified result."
However, I am chagrined to state that I have no idea where I got that idea! I can't find support for that anywhere in the current Standard (although I'm not sure if I missed it).

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.


Repository:
  rCXX libc++

https://reviews.llvm.org/D47344