[BUG] move-assignment operators defaulting to memberwise semantics makes it impossible to cleanup old value

[jbatez]

Consider a smart pointer type for IUnknown that automatically calls IUnknown::Release where appropriate:

ComPtr: type = {
    m_raw: *IUnknown;

    // ...

    operator=: (inout this, move that) = {
        // Take the raw pointer from `that` first.
        // If `&that == &this`, setting it to nullptr
        // first ensures we don't release it.
        new_raw := std::exchange(that.m_raw, nullptr);
        if m_raw { m_raw*.Release(); }
        m_raw = new_raw;
    }

    // ...
}

cppfront generates the following code for the move-assignment operator:

    auto ComPtr::operator=(ComPtr&& that) noexcept -> ComPtr& {
        m_raw = std::move(that).m_raw;
        // Take the raw pointer from `that` first.
        // If `&that == &this`, setting it to nullptr
        // first ensures we don't release it.
        auto new_raw {std::exchange(std::move(that).m_raw, nullptr)}; 
        if (m_raw) {CPP2_UFCS_0(Release, (*cpp2::assert_not_null(m_raw))); }
        m_raw = std::move(new_raw);
        return *this;
    }

cppfront inserted m_raw = std::move(that).m_raw; before my code, meaning there's no way for me to call Release() on the old value.

[jbatez]

I wonder if the general structure of generated move-assignment operators should be something more like:

auto Foo::operator=(Foo&& that) noexcept -> Foo& {
    auto tmp {std::move(that)};
    this->destructor_logic();
    member1 = std::move(tmp.member1);
    member2 = std::move(tmp.member2);
    // ...
    // insert given logic here
    // ...
    return *this;
}

where destructor_logic() is a function that contains the logic explicitly written for the destructor without calling any of the implicit member destructors.

Anecdotally, this would work for all the move-assignment operators I tend to write and would mean I could rely on the auto-generated move-assignment if I only wrote the operator=: (out this, move that) move-constructor.

Alternatively, it could be something like:

auto Foo::operator=(Foo&& that) noexcept -> Foo& {
    if (this != &that) {
        this->destructor_logic();
        member1 = std::move(that.member1);
        member2 = std::move(that.member2);
    }
    // ...
    // insert given logic here
    // ...
    return *this;
}

[JohelEGP]

Some background: operator= always defaults to memberwise.

cppfront already supports assigning out parameters before this' members.
See #369 (comment)'s https://cpp2.godbolt.org/z/j54j65j9q.

meaning there's no way for me to call Release() on the old value.

It's possible if you use an IIFE.

See https://cpp2.godbolt.org/z/e5aEe14b6:

Cpp2

ComPtr: type = {
    m_raw: *IUnknown;

    // ...

    operator=: (inout this, move that) = {
        m_raw = :() -> _ = {
          // Take the raw pointer from `that` first.
          // If `&that == &this`, setting it to nullptr
          // first ensures we don't release it.
          new_raw := std::exchange(that.m_raw, nullptr);
          if m_raw { m_raw*.Release(); }
          return new_raw;
        }();
    }

    // ...
}

Cpp1

    auto ComPtr::operator=(ComPtr&& that) noexcept -> ComPtr& {
        m_raw = [&]() -> auto{auto new_raw {std::exchange(std::move(that).m_raw, nullptr)}; if (m_raw) {CPP2_UFCS_0(Release, (*cpp2::assert_not_null(m_raw)));}return new_raw; }();
        return *this;

          // Take the raw pointer from `that` first.
          // If `&that == &this`, setting it to nullptr
          // first ensures we don't release it.

    }

Normally, that'd require captures.
But notice how it lowers to a Cpp1 lambda with [&] capture.
It's undocumented, AFAIK, but it seems to be supported.

[jbatez]

I suppose it is possible. But definitely unintuitive, and could be awkward if you're dealing with multiple member variables. I'd argue that doing something destructor-ish with the old value is the norm for custom move-assignment operators and you shouldn't have to jump through IIFE hoops to do it.

This isn't the only case where you want to run code before memberwise assignment. Argument validation is another big one, and that'd be useful for constructors as well as assignment operators.

[JohelEGP]

It makes sense to lift the restriction that the memberwise assignments must come first.
There should be no problems, thanks to guaranteed initialization and the required memberwise order.

[JohelEGP]

From commit cdf71bd, generalizing member = value; would solve this.

Is it just because it's too complex?
I think that there is a rule "The first statements of the function must be assignments to the member variables, and they must appear in order. If they don't, then the default assignment will be inserted."

Exactly. I'm considering using the definite-first-use logic for these initializations/assignments too (which would allow that if), and a lot of the machinery for that is already enabled, but I haven't gone that far yet.

Actually, would simply using member _ =; work for your exact use case?

[hsutter]

Thanks @jbatez ! Yes, the new member = _ ; feature I just pushed in cdf71bd does cover this use case... here is your code plus the line m_raw = _ ; at the top of the assignment operator:

ComPtr: type = {
    m_raw: *IUnknown;

    // ...

    operator=: (inout this, move that) = {
        m_raw = _ ;
        // Take the raw pointer from `that` first.
        // If `&that == &this`, setting it to nullptr
        // first ensures we don't release it.
        new_raw := std::exchange(that.m_raw, nullptr);
        if m_raw { m_raw*.Release(); }
        m_raw = new_raw;
    }

    // ...
}

and I checked that the assignment operator now correctly lowers to this:

    auto ComPtr::operator=(ComPtr&& that) noexcept -> ComPtr& {

        // Take the raw pointer from `that` first.
        // If `&that == &this`, setting it to nullptr
        // first ensures we don't release it.
        auto new_raw {std::exchange(std::move(that).m_raw, nullptr)}; 
        if (m_raw) {CPP2_UFCS_0(Release, (*cpp2::assert_not_null(m_raw))); }
        m_raw = std::move(new_raw);
        return *this;
    }

[hsutter]

Regarding implementing the assignment operator in terms of destruction + re-construction... that's possible, but has some issues.

The main one is that it could lead to observable difference in behavior between copy assignment and move assignment. We wouldn't want to implement copy assignment in terms of destroy+construct for efficiency (e.g., a string or vector could not reuse storage, it would have to release all its storage and then reallocate new storage again). But if we did do that for move assignment only, then a user could experience different side effects only for move assignment, such as a de-registration by destructor logic and a re-registration if, say, the object is related to other objects and self-registers itself somewhere.

For completeness: See also GotW #23, but your suggestion as written already avoids most of the pitfalls mentioned there, which is great. 👍

[JohelEGP]

#518 should be closed, too.

[jbatez]

Thanks! m_raw = _; does indeed get the job done.

I still find the opt-out nature of it unintuitive, though, fwiw. And the fact that operator=: (inout this, move that) has different defaults than operator=: (inout this, move other: ComPtr) is surprising. I'm just one data point, though. 🙂