Avoid unnecessary copy of objects. #12158
Conversation
ghost
commented
Jan 11, 2018
- removed unnecessary value arguments.
- removed unnecessary copy initialization.
- removed unnecessary for range copy.
* removed unnecessary value arguments. * removed unnecessary copy initialization. * removed unnecessary for range copy.
|
I ran the tests with Everything seems fine to me. Let me know if this is acceptable or whether there's something I missed. Regards, |
src/rpc/mining.cpp
Outdated
| @@ -103,7 +103,7 @@ UniValue getnetworkhashps(const JSONRPCRequest& request) | |||
| return GetNetworkHashPS(!request.params[0].isNull() ? request.params[0].get_int() : 120, !request.params[1].isNull() ? request.params[1].get_int() : -1); | |||
| } | |||
|
|
|||
| UniValue generateBlocks(std::shared_ptr<CReserveScript> coinbaseScript, int nGenerate, uint64_t nMaxTries, bool keepScript) | |||
| UniValue generateBlocks(const std::shared_ptr<CReserveScript>& coinbaseScript, int nGenerate, uint64_t nMaxTries, bool keepScript) | |||
There was a problem hiding this comment.
Probably more natural to just pass const CReserveScript* than a const shared_ptr.
|
No problem, I can just rebase it when the next merge window happens. |
|
NACK as is. Too much scattered and different type of changes. |
|
@kekimusmaximus Did you use a static analyzer to identify these? Are the fixes exhaustive (covering all instances in the code base)? |
|
@practicalswift Yes, I'm using an unusual setup, since I created a compile command database. But at the crux of it it's 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. I could probably create a pull request for that, but at the end of the day the issues still need to be fixed. |
|
Yes fixes are exhaustive |
|
@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). |
|
@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" 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. |
|
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 :) |
|
All in all, the changes in this PR make sense. Avoiding copies is good. |
|
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. |