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

[Brendan Emery via Phabricator via libcxx-commits]

bemeryesr marked 4 inline comments as done.
bemeryesr added a comment.

Thanks for the feedback! Regarding your OffsetPtr example:

1. My initial code was broken as it was calling the copy assignment operator of `__long` even if `__rep` was a short string. For an offset pointer, the copy assignment operator recalculates the offset to the pointed-to address and so the copied value will be different to the original. Hence, it failed for short strings. So now I perform the copy depending on whether it's a short or a long string.
2. As I think you alluded to, it seems that some of the pointer arithmetic is optimized-away by the compiler, leading to incorrect behaviour. In boost::interprocess::offset_ptr they mentioned this: "Note: using the address of a local variable to point to another address is not standard conforming and this can be optimized-away by the compiler. Non-inlining is a method to remain illegal but correct." To deal with this, we used a couple of tricks that prevent the functions from being inlined and the test passes. Would it make sense to add this code as a unit test as is? Or perhaps we should find a better way to deal with the UB? This is the code that works for me:



  // main.cpp
  //===----------------------------------------------------------------------===//
  //
  // 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 <string>
  
  #include "OffsetPtr.h"
  
  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");
      }
  }



  // OffsetPtr.h
  #ifndef OFFSETPTR_H
  #define OFFSETPTR_H
  
  #include <cassert>
  #include <cstdint>
  #include <iostream>
  #include <iterator>
  #include <type_traits>
  #include <utility>
  
  namespace detail
  {
  using local_difference_type = std::ptrdiff_t;
  
  local_difference_type SubtractPointersBytesImpl(const volatile void* ptr1, const volatile void* ptr2);
  
  // Alias for checking if template types are of the same type after stripping const/volatile qualifiers.
  template <typename T1, typename T2>
  using cv_stripped_types_equal =
      typename std::is_same<typename std::remove_cv<T1>::type, typename std::remove_cv<T2>::type>;
  
  template <typename T1, typename T2, typename = std::enable_if_t<cv_stripped_types_equal<T1, T2>::value>>
  constexpr local_difference_type SubtractPointersBytes(T1* const ptr1, T2* const ptr2)
  {
      return SubtractPointersBytesImpl(ptr1, ptr2);
  }
  
  template <typename T>
  constexpr const T* __attribute__((noinline)) AddOffsetToPointer(const T* const ptr, local_difference_type offset)
  {
      return reinterpret_cast<const T*>(reinterpret_cast<const std::uint8_t*>(ptr) + offset);
  }
  
  template <typename T>
  constexpr const T* __attribute__((noinline)) AddOffsetToPointer(T* const ptr, local_difference_type offset)
  {
      return reinterpret_cast<T*>(reinterpret_cast<std::uint8_t*>(ptr) + offset);
  }
  
  }  // namespace detail
  
  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 = detail::local_difference_type;
      using pointer = OffsetPtr;
      using reference = T&;
  
      constexpr OffsetPtr() = default;
      constexpr OffsetPtr(const OffsetPtr& other)
          : offset(detail::SubtractPointersBytes(other.operator->(), reinterpret_cast<T*>(this)))
      {
      }
  
      constexpr operator OffsetPtr<const T>() const
      {
          return OffsetPtr<const T>{detail::AddOffsetToPointer(reinterpret_cast<const T*>(this), offset)};
      }
  
      constexpr OffsetPtr& operator=(const OffsetPtr& other)
      {
          T* ptr = other.operator->();
          offset = detail::SubtractPointersBytes(ptr, reinterpret_cast<T*>(this));
          return *this;
      }
  
      constexpr explicit OffsetPtr(T* ptr) : offset(detail::SubtractPointersBytes(ptr, reinterpret_cast<T*>(this))) {}
  
      constexpr T* operator->() const
      {
          return const_cast<T*>(detail::AddOffsetToPointer(reinterpret_cast<T*>(const_cast<OffsetPtr*>(this)), offset));
      }
  
      constexpr 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{detail::AddOffsetToPointer(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); }
  };
  
  #endif  // OFFSETPTR_H

  // OffsetPtr.cpp
  #include "OffsetPtr.h"
  
  namespace detail
  {
  
  local_difference_type SubtractPointersBytesImpl(const volatile void* const ptr1, const volatile void* const ptr2)
  {
      return local_difference_type(reinterpret_cast<local_difference_type>(ptr1) -
                                   reinterpret_cast<local_difference_type>(ptr2));
  }
  
  }  // namespace detail



================
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.");
 
----------------
philnik wrote:
> AFAICT pointers don't have to have trivial destructors.
Yeah, you're right. As of C++20, we can use constexpr destructors. Before C++20 they must be trivial in order for basic_string to be constexpr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132149