[PATCH] D35863: Use the allocator's pointers for deallocation in `std::list`

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

Quuxplusone updated this revision to Diff 109678.
Quuxplusone added a comment.

I've updated https://reviews.llvm.org/D35863 to be actually correct AFAICT from my local testing; but I'm not sure what's the most appropriate way to get "fancy allocator" tests into libcxx's test suite. The way I did it locally is here:
https://github.com/Quuxplusone/libcxx/pull/1/files
Basically, I conditionally replace "test_allocator.h" with "test_fancy_allocator.h", and then re-run all the existing tests. "test_fancy_allocator" uses fancy pointers that carry with them a "payload" of the allocated size `n`, and then in `a.deallocate(p,n)` we assert that `p.payload()==n`. A bunch of list tests fail this assertion before this patch, and none fail after this patch.

@ericwf re our Slack conversation yesterday afternoon: Am I correct that changing the fancy pointer to a raw pointer in `iterator::operator->()` like this would be an ABI-breaking change and therefore a non-starter? https://github.com/Quuxplusone/libcxx/commit/c80f4ffad2ad04e8749b162629255b191c896b4f

Even if *that* change were friendly, would I be correct that changing the fancy pointer to a raw pointer in the data member `iterator::__ptr_` would be an ABI-breaking change and therefore a non-starter? (That would also require more drastic surgery in `list::erase(p)`, because we could no longer trust `p.__ptr_` to be deallocatable; we'd have to compute `q = p.__ptr_->__prev_->__next_` and deallocate *that*. I have not attempted to write that patch, because I assume it'd be ABI-breaking anyway.)


https://reviews.llvm.org/D35863

Files:
  include/list

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D35863.109678.patch
Type: text/x-patch
Size: 12289 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170804/b0909cd6/attachment.bin>