Skip to content

Fix/remove some bad std::moves#9172

Merged
Ericson2314 merged 2 commits intoNixOS:masterfrom
tfc:bad-moves
Oct 16, 2023
Merged

Fix/remove some bad std::moves#9172
Ericson2314 merged 2 commits intoNixOS:masterfrom
tfc:bad-moves

Conversation

@tfc
Copy link
Contributor

@tfc tfc commented Oct 16, 2023

Motivation

While being at it, I grepped through the source to identify more positions with bad std::move usage.

I particularly fixed 2 types of moves (grouped in two different commits):

  • bla = *std::move(some_optional); - this moves an optional and then copies its content. The dereference operator must go into the move
  • return std::move(some_stack_var) - these moves are superfluous as they would be moved to the caller anyway, but they even forbid named return value optimization (NRVO) where it would otherwise be applicable.

Context

Priorities

Add 👍 to pull requests you find important.

@Ericson2314
Copy link
Member

Ericson2314 commented Oct 16, 2023

I was preferring the first one because I thought std::move would set the std::optional back to std::nullopt, but I guess it does not.

@Ericson2314 Ericson2314 merged commit 8c049a9 into NixOS:master Oct 16, 2023
@tfc tfc deleted the bad-moves branch October 17, 2023 07:51
@tfc
Copy link
Contributor Author

tfc commented Oct 17, 2023

I was preferring the first one because I thought std::move would set the std::optional back to std::nullopt, but I guess it does not.

No, there is no guarantee on that. It would be nice of the implementer of the move-constructor/assignment-operator to implement it this way, but it is not safe to assume anything on the state of the moved-from object.

tebowy pushed a commit to tebowy/nix that referenced this pull request Jul 11, 2024
Fix/remove some bad std::moves

(cherry picked from commit 8c049a9)
Change-Id: I720273378d2506a13883acee28abd096d099b0d4
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 this pull request may close these issues.

3 participants