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

Modifies the description of self-assignment in the move assignment operator #4961

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Mq-b
Copy link
Contributor

@Mq-b Mq-b commented Feb 29, 2024

I think the description and handling of securing self-assignment is too simplistic.

Not all cases should use self-assignment checking to ensure self-assignment security, and in many cases it is not necessary.

MSVC STL std::unique_ptrstd::shared_ptr

template <class _Dx2 = _Dx, enable_if_t<is_move_assignable_v<_Dx2>, int> = 0>
_CONSTEXPR23 unique_ptr& operator=(unique_ptr&& _Right) noexcept {
    reset(_Right.release());
    _Mypair._Get_first() = _STD forward<_Dx>(_Right._Mypair._Get_first());
    return *this;
}
shared_ptr& operator=(shared_ptr&& _Right) noexcept { // take resource from _Right
    shared_ptr(_STD move(_Right)).swap(*this);
    return *this;
}

class Foo {
    std::string s;
    int i;
};

Foo is inherently self-assignment safe and does not require any additional processing.

  • In short: I don't think we should just describe a simple self-assignment detection method.

Copy link
Contributor

@Mq-b : Thanks for your contribution! The author(s) have been notified to review your proposed change.

Copy link
Contributor

Learn Build status updates of commit eb8b75e:

✅ Validation status: passed

File Status Preview URL Details
docs/cpp/move-constructors-and-move-assignment-operators-cpp.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

@Jak-MS
Copy link
Contributor

Jak-MS commented Feb 29, 2024

@TylerMSFT

  • Can you review this PR?
  • IMPORTANT: When this content is ready to merge, you must add #sign-off in a comment or the approval may get overlooked.

#label:"aq-pr-triaged"
@MicrosoftDocs/public-repo-pr-review-team

@prmerger-automator prmerger-automator bot added the aq-pr-triaged Tracking label for the PR review team label Feb 29, 2024
Copy link
Contributor

@Mq-b : Thanks for your contribution! The author(s) have been notified to review your proposed change.


```cpp
if (this != &other)
{
}
```

There are many other ways, too, such as `copy-and-swap`.
Copy link
Collaborator

@TylerMSFT TylerMSFT Feb 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We wouldn't say that there are other ways unless we were going to describe them all. Instead, : "Another way is copy-and-swap" without the code ticks. But we've also just complicated the situation by providing an alternative without explaining why it might be desirable. I assume you like it because it does the check to make sure the objects aren't the same for you? If so, let's call that out.

Copy link
Contributor Author

@Mq-b Mq-b Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CppCoreGuidelines.

In short, I hope that the description is not so "absolute", we should analyze the specific problem.

We don't have to list too many other ways to write it, but in the words Try to say a little that "there are other ways" is still needed.

@@ -125,14 +125,20 @@ The following procedures describe how to write a move constructor and a move ass
}
```

1. In the move assignment operator, add a conditional statement that performs no operation if you try to assign the object to itself.
1. In the move assignment operator, we need to keep the self-assignment safe, and the simple way is to add a judgment directly.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the original wording. 'judgement' and 'safe' are uncommon and ambiguous in technical writing. And 'judgement' wouldn't translate well, either. Can you restore the original?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the original wording. 'judgement' and 'safe' are uncommon and ambiguous in technical writing. And 'judgement' wouldn't translate well, either. Can you restore the original?

The original description was too absolute.

What I mean here is to ensure "self-assignment-safe", but the original text does not mention this key term.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to say "[...], no operation should be performed if one tries to assign the object to itself. The easiest way is adding a conditional statement that skips operations when both operands refer to the same object."

Copy link
Collaborator

@TylerMSFT TylerMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple comments.

There are many other ways, too, such as `copy-and-swap`.

```cpp
shared_ptr(_Right).swap(*this);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit weird to mention shared_ptr here. And shared_ptr(_Right).swap(*this) is performed in copy assignment, not in move assigment.

In this move-constructors-and-move-assignment-operators-cpp.md, I think we should say MemoryBlock(std::move(other)).swap(*this). BUT we also need to add a swap function together.

@frederick-vs-ja
Copy link

frederick-vs-ja commented Mar 1, 2024

It's curious (or rather old-fashioned) that the whole file mentions neither swap nor exchange. Perhaps it need to be substantially updated.

@Mq-b
Copy link
Contributor Author

Mq-b commented Mar 1, 2024

It's curious (or rather old-fashioned) that the whole file mentions neither swap nor exchange. Perhaps it need to be substantially updated.

Yes!

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

Successfully merging this pull request may close these issues.

4 participants