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.

Sign up for GitHub

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

Jump to bottom

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
+7 −1
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion docs/cpp/move-constructors-and-move-assignment-operators-cpp.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link

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."


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

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

@TylerMSFT TylerMSFT Feb 29, 2024 •
edited
Loading

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 •
edited
Loading

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.


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

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.

```

1. In the conditional statement, free any resources (such as memory) from the object that is being assigned to.

The following example frees the `_data` member from the object that is being assigned to:
Expand Down