Remove redundant copy constructors #17349

1. 
   MarcoFalke commented at 10:20 pm on November 1, 2019: member

   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.
2. refactor: Remove redundant PSBT copy constructor

   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.

   29f8434368
3. bench: Remove redundant copy constructor in mempool_stress fa8919889f
4. MarcoFalke added the label Refactoring on Nov 1, 2019
5. 
   MarcoFalke commented at 10:21 pm on November 1, 2019: member

   cc @achow101 @JeremyRubin. Is there any reason for this that I am missing?
6. 
   theuni commented at 10:26 pm on November 1, 2019: member

   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?
7. 
   MarcoFalke commented at 10:39 pm on November 1, 2019: member

   If a move is not desired, why couldn’t this be achieved with delete and default keywords and proper documentation instead? https://en.cppreference.com/w/cpp/language/move_constructor
8. in src/bench/mempool_stress.cpp:26 in fa8919889f

   22@@ -23,12 +23,6 @@ struct Available {
   23     size_t vin_left{0};
   24     size_t tx_count;
   25     Available(CTransactionRef& ref, size_t tx_count) : ref(ref), tx_count(tx_count){}
   26-    Available& operator=(Available other) {
   

   ---

   ---

   

   promag commented at 10:52 pm on November 1, 2019:

   fa8919889f3c1bd3e2700ecbb56493e3cd1e25ad

   Is this a copy operator? 🤔

9. 
   promag commented at 10:52 pm on November 1, 2019: member

   ACK fa8919889f3c1bd3e2700ecbb56493e3cd1e25ad.
10. 
    JeremyRubin commented at 11:43 pm on November 1, 2019: contributor

    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

11. 
    DrahtBot commented at 1:26 am on November 2, 2019: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #17268 (Epoch Mempool by JeremyRubin)

    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.

12. 
    hebasto commented at 5:56 am on November 2, 2019: member

    ACK fa8919889f3c1bd3e2700ecbb56493e3cd1e25ad, nit s/constructor/operator/ in commit fa8919889f3c1bd3e2700ecbb56493e3cd1e25ad message, as @promag mentioned above.
13. 
    laanwj commented at 8:50 am on November 2, 2019: member

    I also had done this in #16995. There’s this warning that makes people add copy constructors:

    0implicitly-declared ‘PartiallySignedTransaction& PartiallySignedTransaction::operator=(const PartiallySignedTransaction&)’ is deprecated [-Wdeprecated-copy]
    

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

14. 
    jonatack commented at 10:50 am on November 2, 2019: member

    ACK fa8919889f3c1bd3e2700ecbb56493e3cd1e25ad

    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.

     0(pr/17349) $ src/bench/bench_bitcoin
     1WARNING: This is a debug build - may result in slower benchmarks.
     2# Benchmark, evals, iterations, total, min, max, median
     3AssembleBlock, 5, 700, 27.9197, 0.00720689, 0.00938161, 0.00797799
     4Base58CheckEncode, 5, 320000, 117.597, 7.07593e-05, 7.93047e-05, 7.24769e-05
     5...
     6ComplexMemPool, 5, 1, 16.6861, 3.19937, 3.4615, 3.38646
     7...
     8MempoolEviction, 5, 41000, 88.9042, 0.000383962, 0.000517717, 0.000428901
     9...
    10RpcMempool, 5, 40, 12.4578, 0.0601209, 0.0654556, 0.0620714
    

15. MarcoFalke referenced this in commit fba574c908 on Nov 4, 2019
16. MarcoFalke merged this on Nov 4, 2019
17. MarcoFalke closed this on Nov 4, 2019

    ---

18. MarcoFalke deleted the branch on Nov 4, 2019
19. MarkLTZ referenced this in commit 97dfbbb0a0 on Nov 17, 2019
20. jasonbcox referenced this in commit ac76cc89e1 on Sep 11, 2020
21. MarcoFalke locked this on Dec 16, 2021

Contributors
MarcoFalke theuni promag JeremyRubin DrahtBot hebasto laanwj jonatack

Labels
Refactoring