Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Aug 19, 2022

Passing a symbol to std::move that is marked const is a no-op, which can be fixed in two ways:

  • Remove the const, or
  • Remove the std::move

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 19, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25714 (univalue: Avoid std::string copies by MarcoFalke)
  • #24897 ([Draft / POC] Silent Payments by w0xlt)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@maflcko
Copy link
Member Author

maflcko commented Aug 25, 2022

Locally, on aarch64, I see that the BlockToJsonVerbose benchmark is about 2% faster.
Also, for some unknown reason, the BlockToJsonVerboseWrite bench uses 5% less memory?

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK fa87534. Looks good. Good for univalue to support c++11 move optimizations

@fanquake fanquake merged commit 01e1627 into bitcoin:master Aug 31, 2022
@maflcko maflcko deleted the 2208-const-move-📔 branch September 1, 2022 16:13
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 1, 2022
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 18, 2022
fa87534 Fix iwyu (MacroFake)
faad673 Fix issues when calling std::move(const&) (MacroFake)

Pull request description:

  Passing a symbol to `std::move` that is marked `const` is a no-op, which can be fixed in two ways:

  * Remove the `const`, or
  * Remove the `std::move`

ACKs for top commit:
  ryanofsky:
    Code review ACK fa87534. Looks good. Good for univalue to support c++11 move optimizations

Tree-SHA512: 3dc5cad55b93cfa311abedfb811f35fc1b7f30a1c68561f15942438916c7de25e179c364be11881e01f844f9c2ccd71a3be55967ad5abd2f35b10bb7a882edea
@bitcoin bitcoin locked and limited conversation to collaborators Sep 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants