refactor: [tidy] modernize-use-emplace

maflcko commented at 11:22 am on October 4, 2023: member

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 commented at 11:22 am on October 4, 2023: contributor

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.

DrahtBot added the label Refactoring on Oct 4, 2023

hebasto commented at 12:04 pm on October 4, 2023: member

Concept ACK.

hebasto commented at 12:34 pm on October 4, 2023: member

Can we do something with code like that: https://github.com/bitcoin/bitcoin/blob/db7b5dfcc502a8a81c51f56fe753990ae8b3a202/src/bitcoin-cli.cpp#L325 ?

(however, not related to this PR changes)

maflcko commented at 12:57 pm on October 4, 2023: member

Can we do something with code like that:

https://github.com/bitcoin/bitcoin/blob/db7b5dfcc502a8a81c51f56fe753990ae8b3a202/src/bitcoin-cli.cpp#L325 ?

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

DrahtBot added the label Needs rebase on Oct 8, 2023

maflcko force-pushed on Oct 9, 2023

DrahtBot added the label CI failed on Oct 9, 2023

DrahtBot removed the label Needs rebase on Oct 9, 2023

hebasto approved

hebasto commented at 2:17 pm on October 9, 2023: member

ACK dddd734db8140c9ae63cdbefd1f95c9f00ae5b87, I’ve followed clang-tidy’s warnings locally.

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

maflcko commented at 2:45 pm on October 9, 2023: member

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.

DrahtBot removed the label CI failed on Oct 9, 2023

fanquake commented at 10:57 am on October 10, 2023: member

Concept ACK

Sjors commented at 10:03 am on October 11, 2023: member

utACK dddd734db8140c9ae63cdbefd1f95c9f00ae5b87

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.

DrahtBot requested review from fanquake on Oct 11, 2023

maflcko commented at 10:12 am on October 11, 2023: member

@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 approved

aureleoules commented at 10:17 am on October 11, 2023: member

ACK dddd734db8140c9ae63cdbefd1f95c9f00ae5b87

maflcko commented at 10:24 am on October 11, 2023: member

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):

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

DrahtBot added the label CI failed on Oct 11, 2023

maflcko force-pushed on Oct 11, 2023

maflcko commented at 1:05 pm on October 11, 2023: member

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. :smiling_face_with_tear:

Should be trivial to re-ACK.

maflcko marked this as a draft on Oct 11, 2023

maflcko commented at 2:13 pm on October 11, 2023: member

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 commented at 7:31 am on October 12, 2023: member

Example of the CI error:

0clang-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
1script/signingprovider.cpp:371:41: error: unnecessary temporary object created while calling emplace_back [modernize-use-emplace,-warnings-as-errors]
2  371 |     if (track) node.leaves.emplace_back(LeafInfo{.script = std::vector<unsigned char>(script.begin(), script.end()), .leaf_version = leaf_version, .merkle_branch = {}});
3      |

Fix the bug in clang-tidy-17

Is there an upstream issue?

maflcko force-pushed on Oct 12, 2023

maflcko marked this as ready for review on Oct 12, 2023

maflcko commented at 9:02 am on October 12, 2023: member

Just recalled that NOLINTNEXTLINE exists, so I used that.

tidy: modernize-use-emplace fa05a726c2

in src/script/signingprovider.cpp:372 in fac590b9d8 outdated

367@@ -368,6 +368,7 @@ TaprootBuilder& TaprootBuilder::Add(int depth, Span<const unsigned char> script,
368     /* Construct NodeInfo object with leaf hash and (if track is true) also leaf information. */
369     NodeInfo node;
370     node.hash = ComputeTapleafHash(leaf_version, script);
371+    // NOLINTNEXTLINE(modernize-use-emplace)

Sjors commented at 9:08 am on October 12, 2023:

Can you explain why in the source comment? (at least outside of test and fuzz code)

maflcko force-pushed on Oct 12, 2023

Sjors commented at 9:42 am on October 12, 2023: member

re-utACK fa05a726c2

DrahtBot requested review from aureleoules on Oct 12, 2023

DrahtBot requested review from hebasto on Oct 12, 2023

DrahtBot removed the label CI failed on Oct 12, 2023

in src/.bear-tidy-config:12 in fa05a726c2

 7@@ -8,6 +8,8 @@
 8         "src/crypto/ctaes",
 9         "src/leveldb",
10         "src/minisketch",
11+        "src/bench/nanobench.cpp",
12+        "src/bench/nanobench.h",

hebasto commented at 1:07 pm on October 12, 2023:

nit: They might be ordered lexicographically?

hebasto approved

hebasto commented at 1:07 pm on October 12, 2023: member

ACK fa05a726c225dc65dee79367bb67f099ae4f99e6.

TheCharlatan approved

TheCharlatan commented at 12:05 pm on October 16, 2023: contributor

ACK fa05a726c225dc65dee79367bb67f099ae4f99e6

fanquake merged this on Oct 16, 2023

fanquake closed this on Oct 16, 2023

maflcko deleted the branch on Oct 16, 2023

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

Code Coverage

Reviews

Conflicts