Abstract out some of the descriptor Span-parsing helpers #16887

pull sipa wants to merge 3 commits into bitcoin:master from sipa:201909_spanparsing changing 5 files +252 −57

sipa commented at 5:27 PM on September 16, 2019: member

As suggested here: #16800 (comment).

This moves the Span parsing functions out of the descriptor module, making them more easily usable for other parsers (in particular, in preparation for miniscript parsing).
MarcoFalke added the label Refactoring on Sep 16, 2019
MarcoFalke added the label Utils/log/libs on Sep 16, 2019
promag commented at 6:52 PM on September 16, 2019: member

ACK c072446b5c7fbaabcd17494d1b593b1cb5ebc314, verified it's mostly moved code - new code is just forward declarations, license and include guards.
DrahtBot commented at 8:11 PM on September 16, 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:
- #13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by l2a5b1)
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.
in src/util/spanparsing.h:20 in c072446b5c outdated
```
   7 | +
   8 | +#include <string>
   9 | +#include <span.h>
  10 | +
  11 | +/** Parse a constant. If successful, sp is updated to skip the constant and return true. */
  12 | +bool Const(const std::string& str, Span<const char>& sp);
```
laanwj commented at 2:05 PM on September 18, 2019:

with C++ throwing everything into the global namespace by default, I think these functions names (Const, Func, Expr) are too general

MarcoFalke commented at 7:16 PM on September 18, 2019:

Make it a namespace span :) ?

sipa commented at 7:26 PM on September 18, 2019:

I've moved them into a namespace spanparsing, and added some documentation.
laanwj commented at 2:06 PM on September 18, 2019: member

Concept ACK.

Now that these functions are public, would be nice to have unit tests.
Abstract out some of the descriptor Span-parsing helpers 230d43fdbc
Add documenting comments to spanparsing.h 5e69aeec3f
sipa force-pushed on Sep 18, 2019
theStack commented at 6:46 AM on September 20, 2019: member

utACK 230d43f 5e69aee

Now that these functions are public, would be nice to have unit tests.

I would be interested to work on that.
sipa commented at 7:18 PM on September 20, 2019: member

@theStack Be my guest!
theStack commented at 10:16 PM on September 24, 2019: member

@theStack Be my guest!

Unit tests for the Span-parsing helpers are ready, will open a PR for them as soon as this one is merged in (I guess it's not possible to open a PR which depends on another PR, right?)
sipa commented at 10:52 PM on September 24, 2019: member

@theStack You can post a commit, and I can include it in my PR. You can also open a PR against my branch.
fanquake commented at 11:36 PM on September 24, 2019: member

You can post a commit, and I can include it in my PR.

👍 Then we can have the tests alongside the changes in the same PR. @theStack obviously your changes remain attributed to you.
theStack commented at 7:44 AM on September 25, 2019: member

@sipa @fanquake Okay, here we go: https://github.com/theStack/bitcoin/commit/f4db52f932bb18346fda2d3e1bb95290acacc12e
laanwj commented at 8:59 AM on September 25, 2019: member

ACK 5e69aeec3f2a0fafd5e591b7222716f00145761d ACK f4db52f932bb18346fda2d3e1bb95290acacc12e @theStack thanks !
laanwj added the label Waiting for author on Sep 30, 2019
laanwj commented at 5:10 AM on October 3, 2019: member

@sipa if you agree with @theStack's tests, please pull the commit in here
sipa commented at 3:36 PM on October 3, 2019: member

Will do next week.

test: add unit tests for Span-parsing helpers

tests the following four functions:
    - Const() [parse constant]
    - Func()  [parse function]
    - Expr()  [parse expression]
    - Split() [split up a string]

bb36372b8f

sipa commented at 1:12 AM on October 10, 2019: member

Thanks, @theStack; I've included your commit.
fanquake removed the label Waiting for author on Oct 10, 2019
practicalswift commented at 4:02 PM on October 10, 2019: contributor

The parsing heavy functions Const, Func, Expr and Split all seem like good candidates for fuzz testing (in addition to the ordinary unit testing)?

MarcoFalke commented at 4:30 PM on October 10, 2019: member

ACK bb36372b8f2bd675313ae8553ceb61f28c2c1afd

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash fb19a80d964f70a8c4b0e02b5fb1088d6d3c529398648e56d30d9e2f6fb4ba33 -

</details>

MarcoFalke commented at 4:32 PM on October 10, 2019: member

Pretty straightforward move-only commit and then a commit adding tests, going to merge now.
MarcoFalke referenced this in commit befdef8aee on Oct 10, 2019
MarcoFalke merged this on Oct 10, 2019
MarcoFalke closed this on Oct 10, 2019
promag commented at 5:16 PM on October 10, 2019: member

reACK 230d43fdbc41b356700b0d8a6984d69e00279ade, utACK bb36372b8f2bd675313ae8553ceb61f28c2c1afd.
sidhujag referenced this in commit a7e4761106 on Oct 11, 2019
MarcoFalke referenced this in commit 1f6638630e on Oct 16, 2019
deadalnix referenced this in commit f206c4defc on Jul 9, 2020
jasonbcox referenced this in commit 21181ec40c on Jul 9, 2020
jasonbcox referenced this in commit 7dab6aa392 on Jul 9, 2020
jasonbcox referenced this in commit 70e026e888 on Jul 9, 2020
kittywhiskers referenced this in commit 7f4a06c72e on Oct 25, 2021
kittywhiskers referenced this in commit 7180e07170 on Oct 25, 2021
kittywhiskers referenced this in commit 4651c6a28f on Oct 25, 2021
kittywhiskers referenced this in commit 2c6511a649 on Oct 26, 2021
kittywhiskers referenced this in commit 0d19f754a0 on Oct 26, 2021
kittywhiskers referenced this in commit 076d87cd0e on Oct 26, 2021
kittywhiskers referenced this in commit 218fdc9c84 on Oct 28, 2021
kittywhiskers referenced this in commit 164d4e515f on Oct 28, 2021
kittywhiskers referenced this in commit 05287681ed on Oct 28, 2021
kittywhiskers referenced this in commit a191b096d8 on Oct 28, 2021
kittywhiskers referenced this in commit e534b48820 on Oct 29, 2021
pravblockc referenced this in commit 6d9cad5ac1 on Nov 18, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Refactoring Utils/log/libs

Linked (view graph)

#16800 Basic Miniscript support in output descriptors #17113 tests: Add fuzzing harness for descriptor Span-parsing helpers

Abstract out some of the descriptor Span-parsing helpers #16887

Conflicts