Skip to content

Conversation

@laanwj
Copy link
Member

@laanwj laanwj commented Jun 8, 2016

Add error and range-checking parsers for unsigned 32 and 64 bit numbers.
The 32-bit variant is required for parsing sequence numbers from the command line in bitcoin-tx (see #8164 for discussion). I've thrown in the 64-bit variant as a bonus, as I'm sure it will be needed at some
point.

Also adds tests, and updates developer-notes.md.

Add error and range-checking parsers for unsigned 32 and 64 bit numbers.
The 32-bit variant is required for parsing sequence numbers from the
command line in `bitcoin-tx` (see bitcoin#8164 for discussion). I've thrown in
the 64-bit variant as a bonus, as I'm sure it will be needed at some
point.

Also adds tests, and updates `developer-notes.md`.
@laanwj laanwj force-pushed the 2016_06_parseuint branch from 95642d2 to e012f3c Compare June 8, 2016 08:28
@maflcko
Copy link
Member

maflcko commented Jun 8, 2016

utACK e012f3c. I think we also need them in init.cpp

@jonasschnelli
Copy link
Contributor

utACK e012f3c

// Note that strtoul returns a *unsigned long int*, so even if it doesn't report a over/underflow
// we still have to check that the returned value is within the range of an *uint32_t*. On 64-bit
// platforms the size of these types may be different.
return endp && *endp == 0 && !errno &&
Copy link
Member

@sipa sipa Jun 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of checking whether *endp == 0, shouldn't you check whether endp == str.c_str() + str.size() (otherwise you'd accept strings that have a 0 inside)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ParsePrechecks catches this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that solution would be somewhat more elegant. Indeed, ParsePrechecks checks some number-parsing preconditions such as that one.

I'm thinking about (at some point) implementing these functions from scratch instead of calling out to the C API. It wouldn't even be much longer, as there are just too many exceptions. This still doesn't exclude strto*l accepting locale-specific separators, for example. Not a big deal for current uses, but I hate such surprises.

In any case introducing them here (with tests) allows swapping out the implementation later.

Copy link
Member

@sipa sipa Jun 8, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sipa
Copy link
Member

sipa commented Jun 8, 2016

utACK e012f3c

@theuni
Copy link
Member

theuni commented Jun 8, 2016

@laanwj Thanks for grabbing this so quickly! A generic utility function is the way to go for sure.

utACK e012f3c

@laanwj laanwj merged commit e012f3c into bitcoin:master Jun 9, 2016
laanwj added a commit that referenced this pull request Jun 9, 2016
e012f3c util: Add ParseUInt32 and ParseUInt64 (Wladimir J. van der Laan)
codablock pushed a commit to codablock/dash that referenced this pull request Dec 22, 2017
e012f3c util: Add ParseUInt32 and ParseUInt64 (Wladimir J. van der Laan)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
e012f3c util: Add ParseUInt32 and ParseUInt64 (Wladimir J. van der Laan)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants