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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
