[libcxx-commits] [PATCH] D66262: Constrain tuple/unique_ptr move constructors (2899)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

In D66262#2412154 <https://reviews.llvm.org/D66262#2412154>, @zoecarver wrote:

> While attempting to fix the remaining CI build failures I arrived at a problem and I'm not sure what is the best way to proceed. This is the problem:
>
> 1. The "trivial_abi" attribute requires a move constructor or a copy constructor. <https://clang.llvm.org/docs/AttributeReference.html#trivial-abi>
> 2. The standard requires that `unique_ptr`'s "move constructor" doesn't participate in overload resolution unless a requirement is met. This requires a templated "move constructor" so that the function may be disabled.
> 3. A move constructor is not permitted to be templated. From the standard's point of view, I think it's OK for `unique_ptr` not to have a move constructor so long as it is move constructible (which would be the case if it has a templated "move constructor").
>
> I am not super familiar with the `trivial_abi` attribute, so maybe there's some way around this. Any suggestions would be appreciated :)

Oh, so that's the reason for the failures. Basically we're bypassing the `trivial_abi` attribute altogether.

The only way I can *maybe* foresee fixing this in the library would be to specialize `unique_ptr` based on whether the deleter is move constructible (i.e. specialize based on whether the move constructor should be enabled or not). Now, we can't do that without breaking ABI (because it would require changing template parameters to add one we can use `enable_if` on), so we'd have to instead:

1. Inherit from a class like `__unique_ptr_base` which itself does some SFINAE in its template parameters, and has two specializations (one with a move ctor and one without).
2. Use `using __unique_ptr_base::__unique_ptr_base;` inside `std::unique_ptr`.

I'm not 100% sure that works cause I haven't tried it out, but it might be worth trying out. Otherwise, I believe we'd have to discuss with the compiler folks to see whether it makes sense to change the attribute, or we'd need to drop that extension (but that's pretty bad).

Can you try out my suggestion with the base class? I'm thinking about something like:

  template <class _Tp, class _Deleter, bool = is_move_constructible<_Deleter>::value>
  class __unique_ptr_base;
  
  // Move constructible version
  template <class _Tp, class _Deleter, bool = is_move_constructible<_Deleter>::value>
  class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI __unique_ptr_base<_Tp, _Deleter, true> {
      __compressed_pair<pointer, deleter_type> __ptr_;
  
  public:
      _LIBCPP_INLINE_VISIBILITY
      __unique_ptr_base(__unique_ptr_base&& __u) _NOEXCEPT { ... }
  
      // other constructors...
  };
  
  // Non move-constructible version
  template <class _Tp, class _Deleter, bool = is_move_constructible<_Deleter>::value>
  class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI __unique_ptr_base<_Tp, _Deleter, false> {
      __compressed_pair<pointer, deleter_type> __ptr_;
  
  public:
      _LIBCPP_INLINE_VISIBILITY
      __unique_ptr_base(__unique_ptr_base&&) = delete;
  
      // other constructors...
  };
  
  template <class _Tp, class _Dp = default_delete<_Tp> >
  class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr : public __unique_ptr_base<_Tp, _Dp> {
      ...
      using __unique_ptr_base::__unique_ptr_base;
  };


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66262