Remove redundant copy constructors #17349
Conversation
The default (i.e., generated by a compiler) copy constructor does the same things. Also this prevents -Wdeprecated-copy warning for implicitly declared operator= in GCC 9.
|
cc @achow101 @JeremyRubin. Is there any reason for this that I am missing? |
|
This likely has the side-effect of enabling moves, as they would've been implicitly disabled by the explicit assignment operator/copy ctor. Is that desirable? |
|
If a move is not desired, why couldn't this be achieved with |
| @@ -23,12 +23,6 @@ struct Available { | |||
| size_t vin_left{0}; | |||
| size_t tx_count; | |||
| Available(CTransactionRef& ref, size_t tx_count) : ref(ref), tx_count(tx_count){} | |||
| Available& operator=(Available other) { | |||
There was a problem hiding this comment.
Is this a copy operator? 🤔
|
I can't recall exactly but I think it goes something like this: Write code without any stupid generatable stuff. Get compiler error. Add stupid generatable thing manually. Continue coding. Get new compiler error later. Add thing 2 for that error. Thing 2 now makes thing 1 redundant. Compiler doesn't warn that thing 1 is now redundant. git commit |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
I also had done this in #16995. There's this warning that makes people add copy constructors: (in this case the better solution was to remove the custom constructor, but the straightforward "fix" for this warning is to add an explicit copy implementation) (yes, what @JeremyRubin says, it's like warning/error pinball sometimes) |
There was a problem hiding this comment.
ACK fa89198
This appears correct, also afaict per https://en.cppreference.com/w/cpp/language/copy_constructor and https://en.cppreference.com/w/cpp/language/move_constructor. No related build warnings with bench and debug enabled. Regenerated bench data, ran bench_bitcoin and bitcoind.
(pr/17349) $ src/bench/bench_bitcoin
WARNING: This is a debug build - may result in slower benchmarks.
# Benchmark, evals, iterations, total, min, max, median
AssembleBlock, 5, 700, 27.9197, 0.00720689, 0.00938161, 0.00797799
Base58CheckEncode, 5, 320000, 117.597, 7.07593e-05, 7.93047e-05, 7.24769e-05
...
ComplexMemPool, 5, 1, 16.6861, 3.19937, 3.4615, 3.38646
...
MempoolEviction, 5, 41000, 88.9042, 0.000383962, 0.000517717, 0.000428901
...
RpcMempool, 5, 40, 12.4578, 0.0601209, 0.0654556, 0.0620714
fa89198 bench: Remove redundant copy constructor in mempool_stress (MarcoFalke) 29f8434 refactor: Remove redundant PSBT copy constructor (Hennadii Stepanov) Pull request description: I fail to see why people add these copy constructors manually without explanation, when the compiler can generate them at least as good automatically with less code. ACKs for top commit: promag: ACK fa89198. hebasto: ACK fa89198, nit s/constructor/operator/ in commit fa89198 message, as @promag [mentioned](#17349 (comment)) above. jonatack: ACK fa89198 Tree-SHA512: ce024fdb894328f41037420b881169b8b1b48c87fbae5f432edf371a35c82e77e21468ef97cda6f54d34f1cf9bb010235d62904bb0669793457ed1c3b2a89723
Summary: Add new mempool benchmarks for a complex pool (Jeremy Rubin) Pull request description: This PR is related to #17268. It adds a mempool stress test which makes a really big complicated tx graph, and then, similar to mempool_eviction test, trims the size. The test setup is to make 100 original transactions with Rand(10)+2 outputs each. Then, 800 times: we create a new transaction with Rand(10) + 1 parents that are randomly sampled from all existing transactions (with unspent outputs). From each such parent, we then select Rand(remaining outputs) +1 50% of the time, or 1 outputs 50% of the time. Then, we trim the size to 3/4. Then we trim it to just a single transaction. This creates, hopefully, a big bundle of transactions with lots of complex structure, that should really put a strain on the mempool graph algorithms. This ends up testing both the descendant and ancestor tracking. I don't love that the test is "unstable". That is, in order to compare this test to another, you really can't modify any of the internal state because it will have a different order of invocations of the deterministic randomness. However, it certainly suffices for comparing branches. Top commit has no ACKs. --- Backport of Core [[bitcoin/bitcoin#17292 | PR17292]] and [[bitcoin/bitcoin#17349 | PR17349]] (to unbreak [[bitcoin/bitcoin#17292 | PR17292]]) Rationale: On PR17292, struct 'Available' does not follow the rule-of-three with raises a compiler warning that our CI treats with -Werror (it has a user-defined copy-assignment operator without respective copy-constructor and destructor), so I removed it since no deep-copy behavior was required. Noticing that PR17349 did that plus removed a spurious copy-constructor from src/psbt.h, I decided that was reason enough to squash the two PR's together instead of submitting a modified version of each. Test Plan: ninja check check-functional bitcoin-bench -filter=ComplexMemPool Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7414
I fail to see why people add these copy constructors manually without explanation, when the compiler can generate them at least as good automatically with less code.