refactor: [tidy] modernize-use-emplace #28583

maflcko · 2023-10-04T11:22:15Z

Constructing a temporary unnamed object only to copy or move it into a container seems both verbose in code and a strict performance penalty.

Fix both issues via the modernize-use-emplace tidy check.

DrahtBot · 2023-10-04T11:22:17Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, hebasto, TheCharlatan
Concept ACK	fanquake
Stale ACK	aureleoules

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28609 (wallet: Reload watchonly and solvables wallets after migration by achow101)
#28560 (wallet, rpc: FundTransaction refactor by josibake)
#28201 (Silent Payments: sending by josibake)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
#26711 (validate package transactions with their in-package ancestor sets by glozow)
#26326 (net: don't lock cs_main while reading blocks in net processing by andrewtoth)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)
#21283 (Implement BIP 370 PSBTv2 by achow101)

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.

hebasto · 2023-10-04T12:04:07Z

Concept ACK.

hebasto · 2023-10-04T12:34:35Z

Can we do something with code like that:

bitcoin/src/bitcoin-cli.cpp

Line 325 in db7b5df

    
           result.push_back(JSONRPCRequestObj("getnetworkinfo", NullUniValue, ID_NETWORKINFO));

?

(however, not related to this PR changes)

maflcko · 2023-10-04T12:57:48Z

Can we do something with code like that:

bitcoin/src/bitcoin-cli.cpp

Line 325 in db7b5df

result.push_back(JSONRPCRequestObj("getnetworkinfo", NullUniValue, ID_NETWORKINFO));

?

(however, not related to this PR changes)

No. JSONRPCRequestObj is not a constructor, but a function call, so I don't think anything can be done there to improve anything. Also, recall that UniValue::push_back takes an UniValue, which avoids a copy, starting with C++17.

hebasto

ACK dddd734, I've followed clang-tidy's warnings locally.

In your opinion, which value of the IgnoreImplicitConstructors options is better/safer for us?

maflcko · 2023-10-09T14:45:01Z

In your opinion, which value of the IgnoreImplicitConstructors options is better/safer for us?

Not sure why that option exists. Edit: Looking at the tests (clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace-ignore-implicit-constructors.cpp) and the documentation, I could not figure out the use case.

Unrelated: An emplace function is able to call explicit constructors, even though they won't be spelled out explicitly in the source code. A push function is not able to call explicit constructors. However, if the code compiles with a push function, it should also compile with an emplace function.

fanquake · 2023-10-10T10:57:01Z

Concept ACK

Sjors · 2023-10-11T10:03:47Z

utACK dddd734

A push function is not able to call explicit constructors. However, if the code compiles with a push function, it should also compile with an emplace function.

Someone on the internet says that this ability to call explicit constructors can sometimes lead to bugs. But since everything here was first compiled with push_back, that's not an issue. Just something to keep an eye on for new uses of emplace_back.

maflcko · 2023-10-11T10:12:57Z

@hebasto For reference, I edited my previous replies. I realized that UniValue::push_back has a different behavior/signature compared to std::vector<UniValue>::push_back.

aureleoules

ACK dddd734

maflcko · 2023-10-11T10:24:53Z

Someone on the internet says that this ability to call explicit constructors can sometimes lead to bugs. But since everything here was first compiled with push_back, that's not an issue. Just something to keep an eye on for new uses of emplace_back.

Jup, this should only affect new code. Other things to consider: Named args and designated initializers won't be possible with emplace. Also: Theoretically there could be resource leak, when using new to construct an argument passed to emplace on a container that holds smart pointers. But no new instances are introduced in this pull and fixing this issue is unrelated from this pull and already exists on current master (in tests):

src/test/denialofservice_tests.cpp:    vNodes.emplace_back(new CNode{id++,
src/test/fuzz/coinscache_sim.cpp:            caches.emplace_back(new CCoinsViewCache(&bottom, /*deterministic=*/true));
src/test/fuzz/coinscache_sim.cpp:                    caches.emplace_back(new CCoinsViewCache(&*caches.back(), /*deterministic=*/true));

maflcko · 2023-10-11T13:05:18Z

rebased for clang-17. Excluded nanobench for now due to it being C++11, but here it is compiled as C++17, also clang-tidy-17 introduced a new rule that requires C++20. 🥲

Should be trivial to re-ACK.

maflcko · 2023-10-11T14:13:44Z

I guess the only way to fix the CI failure would be:

Go back to clang-tidy-16
Fix the bug in clang-tidy-17
Bump to C++20
Edit: NOLINTNEXTLINE
?

Sjors · 2023-10-12T07:31:52Z

Example of the CI error:

clang-tidy-17 -p=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/script/signingprovider.cpp
script/signingprovider.cpp:371:41: error: unnecessary temporary object created while calling emplace_back [modernize-use-emplace,-warnings-as-errors]
  371 |     if (track) node.leaves.emplace_back(LeafInfo{.script = std::vector<unsigned char>(script.begin(), script.end()), .leaf_version = leaf_version, .merkle_branch = {}});
      |

Fix the bug in clang-tidy-17

Is there an upstream issue?

maflcko · 2023-10-12T09:02:26Z

Just recalled that NOLINTNEXTLINE exists, so I used that.

src/script/signingprovider.cpp

Sjors · 2023-10-12T09:42:05Z

re-utACK fa05a72

hebasto

ACK fa05a72.

hebasto · 2023-10-12T13:07:23Z

src/.bear-tidy-config

+        "src/bench/nanobench.cpp",
+        "src/bench/nanobench.h",


nit: They might be ordered lexicographically?

TheCharlatan

ACK fa05a72

see bitcoin#28583 (comment)

DrahtBot added the Refactoring label Oct 4, 2023

DrahtBot added the Needs rebase label Oct 8, 2023

maflcko force-pushed the 2310-tidy-modernize-use-emplace- branch 2 times, most recently from fa59833 to dddd734 Compare October 9, 2023 13:31

DrahtBot added CI failed and removed Needs rebase labels Oct 9, 2023

hebasto approved these changes Oct 9, 2023

View reviewed changes

DrahtBot removed the CI failed label Oct 9, 2023

DrahtBot mentioned this pull request Oct 10, 2023

wallet: Reload watchonly and solvables wallets after migration #28609

Merged

DrahtBot requested a review from fanquake October 11, 2023 10:03

aureleoules approved these changes Oct 11, 2023

View reviewed changes

DrahtBot added the CI failed label Oct 11, 2023

maflcko force-pushed the 2310-tidy-modernize-use-emplace- branch 3 times, most recently from fa3c80c to fa2876c Compare October 11, 2023 13:04

maflcko marked this pull request as draft October 11, 2023 13:41

maflcko force-pushed the 2310-tidy-modernize-use-emplace- branch from fa2876c to fac590b Compare October 12, 2023 08:02

maflcko marked this pull request as ready for review October 12, 2023 08:09

Sjors reviewed Oct 12, 2023

View reviewed changes

src/script/signingprovider.cpp Show resolved Hide resolved

tidy: modernize-use-emplace

fa05a72

maflcko force-pushed the 2310-tidy-modernize-use-emplace- branch from fac590b to fa05a72 Compare October 12, 2023 09:27

DrahtBot requested review from aureleoules and hebasto October 12, 2023 09:42

DrahtBot removed the CI failed label Oct 12, 2023

hebasto approved these changes Oct 12, 2023

View reviewed changes

maflcko mentioned this pull request Oct 13, 2023

rpc: add hidden getrawaddrman RPC to list addrman table entries #28523

Merged

TheCharlatan approved these changes Oct 16, 2023

View reviewed changes

fanquake merged commit 08ea835 into bitcoin:master Oct 16, 2023

maflcko deleted the 2310-tidy-modernize-use-emplace- branch October 16, 2023 14:57

fanquake mentioned this pull request Oct 17, 2023

refactor: [tidy] modernize-type-traits #28664

Closed

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 21, 2023

Merge bitcoin#28583: refactor: [tidy] modernize-use-emplace

c5ae182

jonatack added a commit to jonatack/bitcoin that referenced this pull request Jan 8, 2024

Remove no-longer-needed NOLINTNEXTLINE since C++20 upgrade

6c4336a

see bitcoin#28583 (comment)

jonatack mentioned this pull request Jan 8, 2024

refactor: use push_back in TaprootBuilder::Add(), rm NOLINTNEXTLINE and comment #29202

Closed

bitcoin locked and limited conversation to collaborators Oct 15, 2024

refactor: [tidy] modernize-use-emplace #28583

refactor: [tidy] modernize-use-emplace #28583

Uh oh!

Conversation

maflcko commented Oct 4, 2023

Uh oh!

DrahtBot commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

hebasto commented Oct 4, 2023

Uh oh!

hebasto commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Oct 10, 2023

Uh oh!

Sjors commented Oct 11, 2023

Uh oh!

maflcko commented Oct 11, 2023

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 11, 2023

Uh oh!

maflcko commented Oct 11, 2023

Uh oh!

maflcko commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Oct 12, 2023

Uh oh!

Uh oh!

Sjors commented Oct 12, 2023

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Oct 4, 2023 •

edited

Loading

hebasto commented Oct 4, 2023 •

edited

Loading

maflcko commented Oct 4, 2023 •

edited

Loading

maflcko commented Oct 9, 2023 •

edited

Loading

maflcko commented Oct 11, 2023 •

edited

Loading

Sjors commented Oct 12, 2023 •

edited

Loading