Avoid unnecessary copy of objects. #12158

1. 
   ghost commented at 7:32 pm on January 11, 2018: none

   • removed unnecessary value arguments.
   • removed unnecessary copy initialization.
   • removed unnecessary for range copy.
2. Avoid unnecessary copy of objects.

   * removed unnecessary value arguments.
   * removed unnecessary copy initialization.
   * removed unnecessary for range copy.

   b460242a36
3. 
   ghost commented at 7:32 pm on January 11, 2018: none

   I ran the tests with make check as well as the functional tests via test/functional/test_runner.py

   Everything seems fine to me.

   Let me know if this is acceptable or whether there’s something I missed.

   Regards,

4. in src/rpc/mining.cpp:106 in b460242a36 outdated

   102@@ -103,7 +103,7 @@ UniValue getnetworkhashps(const JSONRPCRequest& request)
   103     return GetNetworkHashPS(!request.params[0].isNull() ? request.params[0].get_int() : 120, !request.params[1].isNull() ? request.params[1].get_int() : -1);
   104 }
   105 
   106-UniValue generateBlocks(std::shared_ptr<CReserveScript> coinbaseScript, int nGenerate, uint64_t nMaxTries, bool keepScript)
   107+UniValue generateBlocks(const std::shared_ptr<CReserveScript>& coinbaseScript, int nGenerate, uint64_t nMaxTries, bool keepScript)
   

   ---

   ---

   

   ryanofsky commented at 7:48 pm on January 11, 2018:

   Probably more natural to just pass const CReserveScript* than a const shared_ptr.

   ---

   

   unknown commented at 7:54 pm on January 11, 2018:

   will do that
5. 
   ryanofsky commented at 7:50 pm on January 11, 2018: member

   Seems like a good change but probably won’t be able to be merged soon given upcoming release & feature freeze.
6. 
   ghost commented at 7:55 pm on January 11, 2018: none

   No problem, I can just rebase it when the next merge window happens.
7. Remove shared_ptr from mining interface. 7a385e8e72
8. fanquake added the label Refactoring on Jan 13, 2018
9. 
   promag commented at 2:52 pm on February 3, 2018: member

   NACK as is. Too much scattered and different type of changes.
10. 
    practicalswift commented at 4:34 pm on February 8, 2018: contributor

    @kekimusmaximus Did you use a static analyzer to identify these? Are the fixes exhaustive (covering all instances in the code base)?
11. 
    ghost commented at 4:58 pm on February 8, 2018: none

    @practicalswift Yes, I’m using an unusual setup, since I created a compile command database.

    But at the crux of it it’s http://clang.llvm.org/extra/clang-tidy/index.html

    On a more general note, what could be done is have a configure option that has a compilation dry run, where the compiler is set to clang-tidy.

    It would also be useful to have maybe a list of agreed upon checks and committed to the repo. Ideally you’d have the CI run that also.

    I could probably create a pull request for that, but at the end of the day the issues still need to be fixed.

12. 
    ghost commented at 5:01 pm on February 8, 2018: none

    Yes fixes are exhaustive
13. 
    practicalswift commented at 9:15 pm on February 8, 2018: contributor

    @kekimusmaximus Very nice! Did you create the compile command database using Bear (Build EAR)?

    One way to increase the chance of getting this kind of changes accepted is to allow for other to recreate the process you used to identify the issues. That will allow others to verify that the changes are correct and exhaustive. Ideally that would include the commands used (including building the compile database) and the relevant output printed (say posted to a gist linked from here).

14. 
    ghost commented at 10:00 pm on February 8, 2018: none

    @practicalswift Yes I used Bear with verbose make output.

    Bear doesn’t work with bitcoin by default since the buildsystem makes use of libtool. Given I’m a CMake guy, I couldn’t figure out how to get around that so what I did was I hacked Bear.

    Basically I added the following line to /usr//bin/bear.

    250 re.compile(r’^libtool: compile : clang(++)?(-\d+(.\d+){0,2})?$’),

    And of course ran ./configure with CC=“clang” And bear make V=1

    Hope that helps. Ideally this would be integrated in the buildsystem, CMake already provides support for compilation databases.

    I also tried to just pass CC=clang-tidy at configure time but that fails since IIRC in univalue the configure step tries to compile something to see if there’s support and that messes everything up. Again CMake guy.

15. 
    ghost commented at 10:05 pm on February 8, 2018: none

    What I also need to point out if you’re going to try this, since there’s classes of checks that can be automatically fixed if you pass fix.

    It’s not that straight forward, so you need to evaluate the fixes by hand. Sometimes the suggested fixes are wrong for different reasons, I would say 90% of the time is ok except when it’s not :)

16. 
    laanwj commented at 9:15 pm on March 29, 2018: member

    All in all, the changes in this PR make sense. Avoiding copies is good. But due to the large number of changes it’s very hard to review (as @promag says). It would make sense to split commits, or maybe even separate PRs, per category of changes - e.g. those that add std::move, those that add const &, the miscellaneous code style changes etc.
17. 
    laanwj commented at 12:33 pm on April 26, 2018: member

    Going to close this as it seems to be inactive, let me know if you want to pick it up again then I’ll reopen.
18. laanwj closed this on Apr 26, 2018

    ---

19. MarcoFalke added the label Up for grabs on Apr 26, 2018
20. MarcoFalke removed the label Up for grabs on Mar 5, 2019
21. DrahtBot locked this on Dec 16, 2021

Contributors
ghost ryanofsky promag practicalswift laanwj

Labels
Refactoring