LWG 2899 Issue Addressed

[NathanSWard]

Description

This addresses issue #68 by adding a constraint to std::unique_ptr. The move constructors are disabled if the unique_ptr's deleter is not moveable.
The noexcept() specifiers and constraint on the move constructor and assignment operator for std::tuple are already implemented and work correctly.

• I understand README.md. I also understand that acceptance of
  community PRs will be delayed until the test and CI systems are online.
• If this is a feature addition, that feature has been voted into the
  C++ Working Draft.
• Identifiers in product code changes are properly _Ugly as per
  https://eel.is/c++draft/lex.name#3.1 .
• The STL builds successfully and all tests have passed (must be manually
  verified by an STL maintainer before CI is online, leave this unchecked for
  initial submission).
• These changes introduce no known ABI breaks (adding members, renaming
  members, adding virtual functions, changing whether a type is an aggregate
  or trivially copyable, etc.).
• These changes were written from scratch using only this repository and
  the C++ Working Draft as a reference (and any other cited standards).
  If they were derived from a project that's already listed in NOTICE.txt,
  that's fine, but please mention it. If they were derived from any other
  project (including Boost and libc++, which are not yet listed in
  NOTICE.txt), you must mention it here, so we can determine whether the
  license is compatible and what else needs to be done.

[NathanSWard]

The following code is what I used to test std::tuple's noexcept() specifier, the constraints on its move constructor/assignment operators, and the newly added constraints on std::unique_ptr's move constructor/assignment operator.

#include <memory>
#include <tuple>
#include <type_traits>
#include <utility>

struct no_throw {};
struct throws {};

struct test {
    test(const no_throw&) noexcept {}
    test(no_throw&&) noexcept {}
    test& operator=(const no_throw&) noexcept { return *this; }
    test& operator=(no_throw&&) noexcept { return *this; }

    test(const throws&) noexcept(false) {}
    test(throws&&) noexcept(false) {}
    test& operator=(const throws&) noexcept(false) { return *this; }
    test& operator=(throws&&) noexcept(false) { return *this; }
};

struct moveable {};

// used as unique_ptr<int>'s deleter
struct not_move_constructible {
    not_move_constructible() = default;
    not_move_constructible(const not_move_constructible&) = default;
    not_move_constructible& operator=(const not_move_constructible&) = default;
    not_move_constructible& operator=(not_move_constructible&&) = default;

    not_move_constructible(not_move_constructible&&) = delete;

    void operator()(int*) {}
};

struct not_move_assignable {
    not_move_assignable() = default;
    not_move_assignable(const not_move_assignable&) = default;
    not_move_assignable& operator=(const not_move_assignable&) = default;
    not_move_assignable(not_move_assignable&&) = default;
    
    not_move_assignable& operator=(not_move_assignable&&) = delete;

    void operator()(int*) {}
};

int main() {
    using tuple_t = std::tuple<test>;

    /* tuple(UTypes&&... u) */
    static_assert(std::is_nothrow_constructible_v<tuple_t, const no_throw&>);
    static_assert(std::is_nothrow_constructible_v<tuple_t, no_throw&&>);
    static_assert(!std::is_nothrow_constructible_v<tuple_t, const throws&>);
    static_assert(!std::is_nothrow_constructible_v<tuple_t, throws&&>);

    /* tuple(tuple<UTypes...>&& u) */
    static_assert(std::is_nothrow_constructible_v<tuple_t, std::tuple<no_throw>&&>);
    static_assert(!std::is_nothrow_constructible_v<tuple_t, std::tuple<throws>&&>);

    /* tuple& operator=(tuple<UTypes...>&& u) */
    static_assert(std::is_nothrow_assignable_v<tuple_t, std::tuple<no_throw>&&>);
    static_assert(!std::is_nothrow_assignable_v<tuple_t, std::tuple<throws>&&>);

    // pair constructor
    using tuple_2_t = std::tuple<test, test>;
    using pair_no_throw = std::pair<no_throw, no_throw>;
    using pair_throws = std::pair<throws, throws>;

    /* tuple(pair<U1, U2>&& u) */
    static_assert(std::is_nothrow_constructible_v<tuple_2_t, pair_no_throw&&>);
    static_assert(!std::is_nothrow_constructible_v<tuple_2_t, pair_throws&&>);

    /* tuple& operator=(pair<U1, U2>&& u) */
    static_assert(std::is_nothrow_assignable_v<tuple_2_t, pair_no_throw&&>);
    static_assert(!std::is_nothrow_assignable_v<tuple_2_t, pair_throws&&>);

    /* tuple(tuple&& u) */
    // std::tuple<not_moveable> tpl(not_moveable{});
    // std::tuple<not_moveable> moved(std::move(tpl)); <- does not compile (good)
    std::tuple<moveable> tpl(moveable{});
    std::tuple<moveable> moved_tpl(std::move(tpl));

    // unique_ptr is move constructible if deleter is move constructible
    static_assert(std::is_move_constructible_v<std::unique_ptr<int, not_move_assignable>>);
    static_assert(std::is_move_constructible_v<std::unique_ptr<int[], not_move_assignable>>);
    static_assert(!std::is_move_constructible_v<std::unique_ptr<int, not_move_constructible>>);
    static_assert(!std::is_move_constructible_v<std::unique_ptr<int, not_move_constructible>>);

    // unique_ptr is move assignable if deleter is move assignable
    static_assert(std::is_move_assignable_v<std::unique_ptr<int, not_move_constructible>>);
    static_assert(std::is_move_assignable_v<std::unique_ptr<int[], not_move_constructible>>);
    static_assert(!std::is_move_assignable_v<std::unique_ptr<int, not_move_assignable>>);
    static_assert(!std::is_move_assignable_v<std::unique_ptr<int, not_move_assignable>>);

    return EXIT_SUCCESS;
}

[CaseyCarter]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CaseyCarter]

Looks good. I'd be concerned about the lack of positive test coverage (i.e., that unique_ptr is move-meowable when the deleter is_move_meowable_v) were this a new library component, but I know we have enough actual usage of unique_ptr around the test suite to cover it.

I'll approve contingent on this being applied internally with the test bits before it's merged here.

[NathanSWard]

Looks good. I'd be concerned about the lack of positive test coverage (i.e., that unique_ptr is move-meowable when the deleter is_move_meowable_v)

@CaseyCarter that makes sense!
I just added the positive test coverage to my test suite.

[StephanTLavavej]

@CaseyCarter can you explain why we don't need unique_ptr& operator=(const volatile unique_ptr&) = delete; like tuple has? When the constraints SFINAE away, why doesn't the language/compiler attempt to generate copies/moves?

[NathanSWard]

Thanks @StephanTLavavej for catching the constructible to assignable for the move operators. I also went ahead and changed _MyDx to _Dx2 to match the previous style on other SFINAE enable_it_t's

[NathanSWard]

Also updated the tests to specifically check unique_ptr for move constructor and move assignment operator.

[StephanTLavavej]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CaseyCarter]

@CaseyCarter can you explain why we don't need unique_ptr& operator=(const volatile unique_ptr&) = delete; like tuple has? When the constraints SFINAE away, why doesn't the language/compiler attempt to generate copies/moves?

I was confused as well. We were already explicitly deleting unique_ptr's copy constructor and assignment operator - which is unconventional, we'd typically define moves and let the copies be implicitly deleted - and the definitions are in an unconventional place as well at the bottom of the public section of the class bodies after reset. I don't think past us intentionally tried to trick current us, but had we wanted to do so I don't think we could have devised a better way to do it.

[StephanTLavavej]

Oh duh! That resolves my question. (Perhaps we should move the deleted overloads up, but that's not necessary.)

[StephanTLavavej]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephanTLavavej]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).