Modernize code: use std::optional instead of bool/outarg #114
Conversation
| } | ||
|
|
||
| bool ParseScriptNumber(const std::pair<opcodetype, std::vector<unsigned char>>& in, int64_t& k) { | ||
| std::optional<int64_t> ParseScriptNumber(const std::pair<opcodetype, std::vector<unsigned char>>& in) { |
There was a problem hiding this comment.
You missed some call sites, such as:
miniscript/bitcoin/script/miniscript.h
Line 1693 in 8a81eeb
bitcoin/test/fuzz/miniscript.cpp
Outdated
| @@ -77,13 +77,12 @@ struct TestData { | |||
| struct ParserContext { | |||
| typedef CPubKey Key; | |||
|
|
|||
| bool ToString(const Key& key, std::string& ret) const | |||
| std::optional<std::string> ToString(const Key& key, std::string& ret) const | |||
There was a problem hiding this comment.
You need to remove the out argument, too.
There was a problem hiding this comment.
To make a comprehensive list of the missing call sites from these changes:
threshparsingmultiparsingminiscript_stringusage ofToString- the
KeyConverterin the unit tests
From my branch which has this included in Bitcoin Core on which i could actually compile.
Looks good to me otherwise, nice cleanup!
|
Rebased, and addressed comments. I've also made a few additional changes; turning inline "&& (num = ParseScriptNum(...)) &&" conditions into separate lines. |
|
ACK 033238f Checked the diff with my Bitcoin Core branch that i tested quite a lot. I didn't look for more potentially missed called sites too hard. |
13edbef miniscript: mark nodes with duplicate keys as insane (Antoine Poinsot) Pull request description: Based off a rebased #114 ACKs for top commit: sipa: utACK 13edbef Tree-SHA512: 729ff124f1a320f91069cde35b90d313faca012b122255e9b247a05d6e912e5422245dfde0f7881e0205cf8a537d390499cb3f8728e908d8ffe467dd8448eb40
Built on top of #106.