refactor: Move result instead of copy in Solver() #16019

pull promag wants to merge 1 commits into bitcoin:master from promag:2019-05-solver-nit changing 1 files +3 −3

promag commented at 5:40 PM on May 13, 2019: member

Move vector witnessprogram instead of copying. Also drop unnecessary std::vector::clear().
DrahtBot added the label Refactoring on May 13, 2019
in src/script/standard.cpp:155 in a7a912b7d5 outdated
```
 151 | @@ -152,7 +152,6 @@ txnouttype Solver(const CScript& scriptPubKey, std::vector<std::vector<unsigned
 152 |          return TX_MULTISIG;
 153 |      }
 154 |  
 155 | -    vSolutionsRet.clear();
```
MarcoFalke commented at 6:20 PM on May 13, 2019:

The clear here is for documentation purposes (and clearing an empty vector is free anyway)

promag commented at 9:09 PM on May 13, 2019:

If it's documentation then maybe change to assert(vSolutionsRet.empty())?

Empact commented at 3:13 AM on May 17, 2019:

Would suggest keeping the line - it enforces an invariant against future change, and would be good to consider it separately if we are to change it. assert could be overkill given this is basically a failure case anyway.

jb55 commented at 3:04 PM on May 18, 2019:

yeah but assert would catch a situation where you missed a return higher up which is something that would definitely be good to catch right away.

promag commented at 1:34 PM on May 20, 2019:

Now using assert(...). It's an unrelated change and I don't mind dropping it but either way assert() looks better to me.
MarcoFalke commented at 6:20 PM on May 13, 2019: member

How much are the speedups?
promag commented at 9:08 PM on May 13, 2019: member

@MarcoFalke didn't measured, but think there's no noticeable speedup. I'm just doing the same as L120.
refactor: Move result instead of copy in Solver() 1c12a262d7
promag force-pushed on May 20, 2019
laanwj commented at 10:18 AM on May 29, 2019: member

I'd prefer to keep this code simpler if there's no noticable speedup.
promag commented at 6:06 PM on May 29, 2019: member

@laanwj not even replacing clear() with the assert()?
promag closed this on Jun 3, 2019
promag deleted the branch on Jun 3, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels