[libcxx-commits] [PATCH] D132149: [String] Allow fancy pointer as pointer type of basic_string allocator

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added subscribers: ldionne, philnik.
philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.

Thanks for working on this! I think in general you take the right approach, but currently the implementation is broken. For example

  //===----------------------------------------------------------------------===//
  //
  // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
  // See https://llvm.org/LICENSE.txt for license information.
  // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
  //
  //===----------------------------------------------------------------------===//
  
  #include <cassert>
  #include <cstddef>
  #include <string>
  
  template <class T>
  class alignas(std::max(alignof(T), alignof(std::ptrdiff_t))) OffsetPtr {
    std::ptrdiff_t offset;
  
  public:
    using value_type        = T;
    using iterator_category = std::random_access_iterator_tag;
    using difference_type   = std::ptrdiff_t;
    using pointer           = OffsetPtr;
    using reference         = T&;
  
    OffsetPtr() = default;
    OffsetPtr(const OffsetPtr& other) : offset(other.operator->() - reinterpret_cast<T*>(this)) {}
    operator OffsetPtr<const T>() const { return OffsetPtr<const T>{reinterpret_cast<const T*>(this) + offset}; }
  
    OffsetPtr& operator=(const OffsetPtr& other) {
      T* ptr = other.operator->();
      offset = ptr - reinterpret_cast<T*>(this);
      return *this;
    }
  
    explicit OffsetPtr(T* ptr) : offset(ptr - reinterpret_cast<T*>(this)) {}
    T* operator->() const { return reinterpret_cast<T*>(const_cast<OffsetPtr*>(this)) + offset; }
    T& operator[](size_t i) { return operator->()[i]; }
  
    static OffsetPtr pointer_to(T& e) { return OffsetPtr(&e); }
  
    friend OffsetPtr operator+(const OffsetPtr& lhs, size_t rhs) { return OffsetPtr{lhs.operator->() + rhs}; }
  };
  
  static_assert(sizeof(OffsetPtr<int>) == 8);
  
  template <class T>
  class OffsetPtrAllocator {
  public:
    using value_type = T;
    using pointer    = OffsetPtr<T>;
  
    pointer allocate(size_t n) { return pointer(std::allocator<T>{}.allocate(n)); }
  
    void deallocate(pointer ptr, size_t n) { std::allocator<T>{}.deallocate(ptr.operator->(), n); }
  };
  
  int main() {
    using string = std::basic_string<char, std::char_traits<char>, OffsetPtrAllocator<char>>;
    string str3 = "almost long string";
    string str4 = std::move(str3);
    assert(str4 == "almost long string");
  }

was rejected before this patch, but now results in wrong results. The `assert(str4 == "almost long string")` fails with your current patch. While the `OffsetPtr` has undefined behaviour, it works and is used in a slightly different style as `boost::interprocess::offset_ptr` in practice. You could also use a pointer and add additional state to avoid the UB. Also, we have rejected these kinds of pointers until now, so we can still define the ABI. We should at least consider whether we want to use the alternate layout for fancy pointers. @ldionne WDYT?



================
Comment at: libcxx/include/string:691
                   "Allocator::value_type must be same type as value_type");
+    static_assert((is_trivially_destructible<pointer>::value), "pointer type must have a trivial destructor.");
 
----------------
AFAICT pointers don't have to have trivial destructors.


================
Comment at: libcxx/test/libcxx/strings/basic.string/allocator_with_fancy_pointer.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Instead of having a single test, you should add the allocator to `test_allocator.h` and add an instantiation to all the string tests to have proper coverage.


================
Comment at: libcxx/test/libcxx/strings/basic.string/allocator_with_fancy_pointer_non_trivial_destructor.fail.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
This should be a `.verify.cpp`, not a `.fail.cpp`.


================
Comment at: libcxx/test/libcxx/strings/basic.string/allocator_with_fancy_pointer_non_trivial_destructor.fail.cpp:15
+
+#ifndef _LIBCPP_CXX03_LANG
+
----------------
You can just add `// UNSUPPORTED: c++03` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132149