Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in move assignment #16

Closed
serges147 opened this issue Aug 19, 2024 · 1 comment · Fixed by #17
Closed

Bug in move assignment #16

serges147 opened this issue Aug 19, 2024 · 1 comment · Fixed by #17
Assignees

Comments

@serges147
Copy link
Collaborator

Here is current code of the move assignment:

    auto operator=(Node&& other) noexcept -> Node&
    {
        moveFrom(other);
        return *this;
    }

The moveFrom private helper is built in assumption (with corresponding assert!) that this node is un-linked b/c it...

  1. either never been linked - just being constructed (see move ctor)
  2. or, were properly removed first from a tree, and now ready to be reassigned (see move assignment operator)

So, for the case # 2 (operator=(Node&&) there should be also conditional node removal (in case it's linked to a tree), like this:

    auto operator=(Node&& other) noexcept -> Node&
    {
        if (isLinked())
        {
            remove();
        }
        moveFrom(other);
        return *this;
    }

Such fix requires refactoring of Node::remove methods to make them non-static, and not requiring initial origin argument, so that method prototype will be like this:

    void remove() const noexcept
    {
        removeImpl(this);
    }
    void remove() noexcept
    {
        removeImpl(this);
        unlink();
    }

, and private static removeImpl will be very close to current algorithm with minor changes so that neither initial origin nor root knowledge is required (deduced from context instead).

@serges147 serges147 self-assigned this Aug 21, 2024
@pavel-kirienko pavel-kirienko linked a pull request Aug 24, 2024 that will close this issue
@serges147
Copy link
Collaborator Author

Resolved by PR #17 - closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant