WIP: refactor: Use our own integer parsing/formatting everywhere

laanwj commented at 8:43 pm on November 5, 2019: member

This replaces direct use of (often locale dependent) C library functions with our own integer parsing and formatting functions., as the developer notes recommend.

For parsing this introduces parse error checking that was not there before. In many cases it’s clear what to do on failure, but sometimes it’s not. It also adds explicitly sized types everywhere. Please pay attention to this while reviewing.

See individual commits for details.

This leaves the following “known violations”:

"src/bitcoin-tx.cpp.*trim_right" boost function, out of scope
"src/dbwrapper.cpp:.*vsnprintf" copied function, the comment addresses this: https://github.com/bitcoin/bitcoin/blob/master/src/dbwrapper.cpp#L19
"src/httprpc.cpp.*trim" another boost function, out of scope
"src/torcontrol.cpp:.*strtol" octal parsing, out of scope
"src/util/strencodings.cpp:.*strtol" part of ParseInt32 and ParseUInt32, gated by our own checks
"src/util/strencodings.cpp:.*strtoul" part of ParseInt64 and ParseUInt64

util: Add error handling to sequence id in bitcoin-tx

Also removes a use of a locale-dependent function.

f7f2472d16

refactor: Replace use of stoul in dbwrapper.cpp

Replace the direct use of `stoul` in dbwrapper.cpp. This removes
a locale dependent exception.

df211cb3f0

refactor: Replace use of atoi in CleanupBlockRevFiles

Replace the use of atoi in CleanupBlockRevFiles with ParseUInt32 to
handle parse errors and remove direct use of a locale-dependent function.

f47450e1dc

refactor: Replace ad-hoc validity check and atoi in rpcconsole 5cdaf4ad65

refactor: Replace use of snprintf with strprintf in dbwrapper_tests

Replace use of function `snprintf` with our own, locale-independent
function `strprintf`.

812c7327cc

refactor: Replace use of strtol in rest.cpp

Replace the use of locale-dependent function strtol with our own
error-checking integer parsing function `ParseUInt32`.

a8f7502d21

refactor: Replace use of atoi for bool parsing in system.cpp

Replace use of atoi with `ParseInt64`, whose output is at least
well-defined. Concretely, the behaviour should not change.

d78e7be423

refactor: Replace use of atoi in torcontrol

Replace use of `atoi` in torcontrol for parsing reply code with
`ParseUInt32`, adding error checking.

Also, change type of `code` to `uint32_t`, it is never negative does not
need to be a signed integer.

a8ded10262

refactor: Remove now-unused atoi from util 6992e0d387

refactor: Replace use of atoi64 in core_read

Replace use of atoi64 and circuitois way to test if something is a
valid 64-bit integer, with the use of our own function `ParseInt64`.

e4aa1f77b1

refactor: Replace use of atoi64 in rpc/mining.cpp

Replace the use of atoi64 with error and bound checking
`ParseUInt32` - after all, the result is being assigned to an
unsigned int.

b7315cb511

refactor: Replace use of atoi64 in moneystr.cpp

Replace the direct use of atoi64 with error-checking
ParseInt64.

2fcdd169cf

laanwj added the label Refactoring on Nov 5, 2019

refactor: Replace use of atoi64 in ArgsManager::GetArg

Replace the use of atoi64 with error-checking ParseInt64.

Unfortunately, there is currently no way to return a parse error,
so return the default in that case.

afb58090ba

refactor: Replace use of atoi64 in wallet ReadOrderPos

Replace use of `atoi64` with error-checking `ParseInt64`.

87a3f13dfe

refactor: Replace use of atoi64 in timesmart unserialize

Replace direct use of atoi64 with ParseUInt64 (as the result is an
unsigned timestamp).

f4218c26bb

refactor: Remove no-longer-used atoi64 from util 88e8307540

laanwj force-pushed on Nov 5, 2019

practicalswift commented at 8:58 pm on November 5, 2019: contributor

Wow! That was quick! :)

Strong Concept ACK

Thanks for taking the time to get rid of those once and for all :)

laanwj commented at 9:39 pm on November 5, 2019: member

Test failure in util_tests/util_ChainMerge explained:

This test passes -notestnet=1 and -noregtest=1 to the argument parser.
Somehow, this ends up as InterpretBool("1testnet=1")
atoi returns 1 in this case (evaluating to true) because it stops parsing at the first non-digit, our ParseInt64 fails however and returns false. This causes the test to exit with a different hash.

Do we really support -notestnet=1 and -noregtest=1? I’ve never seen this syntax.

ryanofsky commented at 9:51 pm on November 5, 2019: member

Do we really support -notestnet=1 and -noregtest=1? I’ve never seen this syntax.

I’ll take a look at this. We could make -notestnet and testnet=0 style options into errors, though this PR is probably not the best place to do it, and maybe there is some use case if you have a configuration file that normally sets testnet, but you want to override it on the command line and use the settings on regtest instead or something.

Any case, will first debug and see what causes reported 1testnet=1

laanwj commented at 9:54 pm on November 5, 2019: member

We ideally need a way to signal parse errors from GetArg functions, there’s a similar situation in ArgsManager::GetArg(int64). Maybe they could return Optional. Out of scope for this PR anyhow, it only needs to do the sane thing here.

some use case if you have a configuration file that normally sets testnet, but you want to override it on the command line and use the settings on regtest instead or something.

In that case couldn’t you simply do -testnet=0 or -regtest=0? I don’t see the point of -noX with a value.

though this PR is probably not the best place to do it

I agree, I’m tempted to just drop that commit, though it means atoi(string) sticks around.

ryanofsky referenced this in commit 3645e4ca00 on Nov 5, 2019

Empact commented at 10:35 pm on November 5, 2019: member

Concept ACK https://github.com/bitcoin/bitcoin/pull/17385/commits/88e83075406493304c3e33c9c1b6a451322ce634

f: add TODO about returning parse error

This is way too lenient.

55ba3887d0

ryanofsky commented at 10:44 pm on November 5, 2019: member

Discussion seems somewhat offtopic here, maybe discuss argument parsing behavior more in #16545.

We ideally need a way to signal parse errors from GetArg functions, there’s a similar situation in ArgsManager::GetArg(int64). Maybe they could return Optional. Out of scope for this PR anyhow, it only needs to do the sane thing here.

I could be wrong but I think it would actually be less work, and friendlier to both users and developers to validate settings on initialization rather than use. #16545 is intended to move in this direction.

some use case if you have a configuration file that normally sets testnet, but you want to override it on the command line and use the settings on regtest instead or something.

In that case couldn’t you simply do -testnet=0 or -regtest=0? I don’t see the point of -noX with a value.

For int and bool arguments, I think it’s generally reasonable to treat -noarg and -noarg=1 the same as -arg=0.

I agree, I’m tempted to just drop that commit, though it means atoi(string) sticks around.

I think it would be best to save both d78e7be42300c4a313ad94aae998b04c25881cba and afb58090ba4f834aa40730512f2485c09f33f32e for another PR and not include them here. These are user facing changes which could use release notes and better test coverage.

laanwj commented at 10:56 pm on November 5, 2019: member

I think it would be best to save both d78e7be and afb5809 for another PR and not include them here.

I’m fine with adding a release not if you think this is a significant user-facing change, and add a test for these two little functions, but I’d really prefer not to keep either the current behavior or code.

DrahtBot commented at 11:19 pm on November 5, 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:

#17050 (tests: Add fuzzing harnesses for functions parsing scripts, numbers, JSON and HD keypaths (bip32) by practicalswift)
#15934 (Merge settings one place instead of five places by ryanofsky)

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.

ryanofsky commented at 0:22 am on November 6, 2019: member

I’m fine with adding a release not if you think this is a significant user-facing change, but I’d really prefer not to keep either the current behavior or code.

My thinking is that there probably shouldn’t be user-facing changes in a PR that’s supposed to be a refactor, and that this PR is substantial enough with 17 commits, that pulling out 2 of them to a dedicated PR wouldn’t make it worse off.

But the user-facing changes are maybe more minor than I would have thought. With your two commits arguments such as "5 " or " 5" or "5." which are currently parsed as 5/true will now be parsed as 0/false. This applies to numeric arguments with leading whitespace or trailing characters.

laanwj commented at 0:33 am on November 6, 2019: member

My thinking is that there probably shouldn’t be user-facing changes in a PR that’s supposed to be a refactor, and that this PR is substantial enough with 17 commits, that pulling out 2 of them to a dedicated PR wouldn’t make it worse off.

Many of the changes in this PR make a subtle difference in the case of invalid input, due to adding strict parse error checking (I mention this in the OP). Maybe I shouldn’t call it a refactor. But it’s not really a bugfix or feature either.

and that this PR is substantial enough with 17 commits, that pulling out 2 of them to a dedicated PR wouldn’t make it worse off.

Sure. But remove any and the cleanup in util.cpp/util.h isn’t possible. Which was one of my motivations to do this in the first place.

ryanofsky commented at 4:41 am on November 6, 2019: member

I’ll review this tomorrow. I think all the commits here that remove uses of C functions to parse strings which are supposed to have a rigid format (numbers in filenames, test strings, descriptors, database properties) are great, and it’s great to clean up this code and avoid misusing these C functions.

But while I’ll need to review the PR more carefully to be sure, I think the few changes here that can actually affect users should probably be done more carefully. For example, instead of taking a strangely formatted boolean value that was previously interpreted as true and now silently interpreting it as false, actually add proper error handling and make it an error to specify a badly formatted value. Or if this is too much work, at least log a warning about badly formatted values. And if there has to be a visible change in behavior, document what the change is in release notes. And if the change is too niche to document in release notes, at least say what the change of behavior is in the commit message.

Alternately, it it’s possible to drop a few commits in this PR to make it a boring plain refactoring, that would be great, because we’d still be eliminating the majority of misused C function calls, and just adding one or two items to your “known violations” list that could be addressed in followups.

laanwj referenced this in commit 224c19645f on Nov 6, 2019

laanwj closed this on Nov 6, 2019

laanwj commented at 9:06 am on November 6, 2019: member

Closing this until the arguments manager has a way to report errors to the user. I agree that needs to be done. It’d be fairly easy to add error return value to the ArgParse functions for bool and int, however there are so many call sites… and they all assume fast and loose error handling (ie none).

I could be wrong but I think it would actually be less work, and friendlier to both users and developers to validate settings on initialization rather than use. #16545 is intended to move in this direction.

This would definitely be better. If you do this, please change the parsing function as well.

in src/dbwrapper.cpp:210 in df211cb3f0 outdated

202@@ -203,7 +203,12 @@ size_t CDBWrapper::DynamicMemoryUsage() const {
203         LogPrint(BCLog::LEVELDB, "Failed to get approximate-memory-usage property\n");
204         return 0;
205     }
206-    return stoul(memory);
207+    uint64_t ret;
208+    if (ParseUInt64(memory, &ret)) {
209+        return ret;
210+    } else {
211+        return 0;

ryanofsky commented at 5:32 pm on November 6, 2019:

In commit “refactor: Replace use of stoul in dbwrapper.cpp” (df211cb3f02e1eb05eb6289af1688afd96660ca7)

Should probably add log print here similar to above:

0LogPrint(BCLog::LEVELDB, "Failed to parse approximate-memory-usage property\n");

There is also some risk here that this change could make DynamicMemoryUsage now return 0 in cases where it returned nonzero before (if property string has leading or trailing whitespace or other trailing characters), but logging should make it obvious if this is ever the case.

laanwj commented at 7:16 pm on November 6, 2019:

I thought about assert(0) here, but then reconsidered, because it’s not important enough to crash the program. Will add a one-time log message.

in src/init.cpp:650 in f47450e1dc outdated

643@@ -644,9 +644,10 @@ static void CleanupBlockRevFiles()
644     // zero by walking the ordered map (keys are block file indices) by
645     // keeping a separate counter.  Once we hit a gap (or if 0 doesn't exist)
646     // start removing block files.
647-    int nContigCounter = 0;
648+    uint32_t nContigCounter = 0;
649     for (const std::pair<const std::string, fs::path>& item : mapBlockFiles) {
650-        if (atoi(item.first) == nContigCounter) {
651+        uint32_t value;
652+        if (ParseUInt32(item.first, &value) && value == nContigCounter) {

ryanofsky commented at 5:42 pm on November 6, 2019:

In commit “refactor: Replace use of atoi in CleanupBlockRevFiles” (f47450e1dcbd4e88efd8e69924da9d20930b399c)

Note: The behavior change here makes this code slightly more robust than before. Now if there are extra files like blkABC they will just be deleted and not throw off the count of expected block files that are supposed to be kept. Throwing off the count would cause files we actually want to keep to be deleted.

in src/qt/rpcconsole.cpp:226 in 5cdaf4ad65 outdated

221@@ -222,10 +222,12 @@ bool RPCConsole::RPCParseCommandLine(interfaces::Node* node, std::string &strRes
222                                 UniValue subelement;
223                                 if (lastResult.isArray())
224                                 {
225-                                    for(char argch: curarg)
226-                                        if (!IsDigit(argch))

ryanofsky commented at 5:47 pm on November 6, 2019:

In commit “refactor: Replace ad-hoc validity check and atoi in rpcconsole” (5cdaf4ad65b578c86ee21f9b7a7a48473a2ca735)

Slight change of behavior here with looser parsing since this now accepts + or - signed values. But this seems fine and doesn’t need additional documentation (though it could be mentioned in the commit message) since there is immediate feedback in case of an error.

in src/bitcoin-tx.cpp:259 in f7f2472d16 outdated

254@@ -255,8 +255,11 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu
255 
256     // extract the optional sequence number
257     uint32_t nSequenceIn = CTxIn::SEQUENCE_FINAL;
258-    if (vStrInputParts.size() > 2)
259-        nSequenceIn = std::stoul(vStrInputParts[2]);
260+    if (vStrInputParts.size() > 2) {
261+        if (!ParseUInt32(vStrInputParts[2], &nSequenceIn)) {

ryanofsky commented at 5:48 pm on November 6, 2019:

In commit “util: Add error handling to sequence id in bitcoin-tx” (f7f2472d16be78bf9c2a7c85e5d40b3b78ef214a)

Slight change of behavior here with stricter parsing. But this seems fine and doesn’t need additional documentation (though it could be mentioned in the commit message) since there is immediate feedback in case of an error.

in src/rest.cpp:131 in a8f7502d21 outdated

126@@ -127,8 +127,8 @@ static bool rest_headers(HTTPRequest* req,
127     if (path.size() != 2)
128         return RESTERR(req, HTTP_BAD_REQUEST, "No header count specified. Use /rest/headers/<count>/<hash>.<ext>.");
129 
130-    long count = strtol(path[0].c_str(), nullptr, 10);
131-    if (count < 1 || count > 2000)
132+    uint32_t count;
133+    if (!ParseUInt32(path[0], &count) || count < 1 || count > 2000)

ryanofsky commented at 5:52 pm on November 6, 2019:

In commit “refactor: Replace use of strtol in rest.cpp” (a8f7502d21d8bbe0934c0c8674cb262d2e913c33)

Slight change of behavior here with stricter parsing: trailing characters no longer allowed for example. But this seems fine and doesn’t need additional documentation (though it could be mentioned in the commit message) since there is immediate feedback in case of an error.

in src/torcontrol.cpp:149 in a8ded10262 outdated

145@@ -146,7 +146,10 @@ void TorControlConnection::readcb(struct bufferevent *bev, void *ctx)
146         if (s.size() < 4) // Short line
147             continue;
148         // <status>(-|+| )<data><CRLF>
149-        self->message.code = atoi(s.substr(0,3));
150+        if (!ParseUInt32(s.substr(0,3), &self->message.code)) {

ryanofsky commented at 5:59 pm on November 6, 2019:

In commit “refactor: Replace use of atoi in torcontrol” (a8ded10262c16778aaa87db0d8ba407e55ea0100)

Note: this change looks like an improvement assuming 3 character torcontrol status code is not allowed to contain whitespace or non-digit characters, which seems reasonable but I haven’t verified this. Error handling seems consistent with other handling done this function of just ignoring lines with unexpected formatting.

laanwj commented at 7:17 pm on November 6, 2019:

Silently ignoring the issue isn’t nice (it makes things hard to debug). I’ll add a log message here.

in src/core_read.cpp:58 in e4aa1f77b1 outdated

55         {
56             // Empty string, ignore. (boost::split given '' will return one word)
57         }
58-        else if (std::all_of(w->begin(), w->end(), ::IsDigit) ||
59-            (w->front() == '-' && w->size() > 1 && std::all_of(w->begin()+1, w->end(), ::IsDigit)))
60+        else if (ParseInt64(*w, &n))

ryanofsky commented at 6:04 pm on November 6, 2019:

In commit “refactor: Replace use of atoi64 in core_read” (e4aa1f77b16c248488cc88b661bed69168c0f9e5)

Parsing here seems a little looser than before, because ParseScript will now accept signed numbers beginning with +, while it previously only accepted numbers beginning with -. I’m assuming this is reasonable behavior. ParseScript appears to only actually be called in tests and by bitcoin-tx.

laanwj commented at 7:12 pm on November 6, 2019:

ParseIntXX accepts signed numbers beginning with +? whoops, I don’t think this was intentional. Thanks for catching this, I think that needs to be documented.

Agree it’s not really problematic in this specific case, but will re-add the “starts with digit or -” check just in case.

in src/rpc/mining.cpp:497 in b7315cb511 outdated

491@@ -492,7 +492,11 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
492             std::string lpstr = lpval.get_str();
493 
494             hashWatchedChain = ParseHashV(lpstr.substr(0, 64), "longpollid");
495-            nTransactionsUpdatedLastLP = atoi64(lpstr.substr(64));
496+            uint32_t temp;
497+            if (!ParseUInt32(lpstr.substr(64), &temp)) {
498+                throw JSONRPCError(RPC_INVALID_PARAMETER, "could not parse lpval");

ryanofsky commented at 6:07 pm on November 6, 2019:

In commit “refactor: Replace use of atoi64 in rpc/mining.cpp” (b7315cb5114477ede6ebb4e4c2303537dcb647b4)

Slight change of behavior here with stricter parsing. For example no whitespace or trailing characters allowed anymore. But this seems fine and doesn’t need additional documentation (though it could be mentioned in the commit message) since there is immediate feedback in case of an error.

in src/util/moneystr.cpp:72 in 2fcdd169cf outdated

67@@ -68,7 +68,10 @@ bool ParseMoney(const char* pszIn, CAmount& nRet)
68         return false;
69     if (nUnits < 0 || nUnits > COIN)
70         return false;
71-    int64_t nWhole = atoi64(strWhole);
72+    int64_t nWhole;
73+    if (!ParseInt64(strWhole, &nWhole)) {

ryanofsky commented at 6:12 pm on November 6, 2019:

In commit “refactor: Replace use of atoi64 in moneystr.cpp” (2fcdd169cfcb1c0fcfb9d8a31fd9459d39a2dfa8)

I might be missing something looking at this code, but it seems like a possible bug if empty string would have been parse successfully as 0 before but now it will fail to parse. Otherwise behavior seems unchanged since strWhole can only contain digit characters.

laanwj commented at 7:14 pm on November 6, 2019:

Seems we need a test for ParseMoney that exercises that!

in src/wallet/wallet.h:203 in 87a3f13dfe outdated

199@@ -200,12 +200,10 @@ typedef std::map<std::string, std::string> mapValue_t;
200 
201 static inline void ReadOrderPos(int64_t& nOrderPos, mapValue_t& mapValue)
202 {
203-    if (!mapValue.count("n"))
204+    if (!mapValue.count("n") || !ParseInt64(mapValue["n"], &nOrderPos))

ryanofsky commented at 6:17 pm on November 6, 2019:

In commit “refactor: Replace use of atoi64 in wallet ReadOrderPos” (87a3f13dfeff735aadb1091183f5269c9ab1a118)

Note: Slightly stricter parsing is done here (no whitespace or trailing characters allowed) but this should be fine since string just comes from calling i64tostr. Could potentially log or return an error though if parsing fails, since it would be an indication of corruption.

in src/wallet/wallet.h:418 in f4218c26bb outdated

413@@ -414,7 +414,12 @@ class CWalletTx
414         }
415 
416         ReadOrderPos(nOrderPos, mapValue);
417-        nTimeSmart = mapValue.count("timesmart") ? (unsigned int)atoi64(mapValue["timesmart"]) : 0;
418+        uint64_t temp;
419+        if (mapValue.count("timesmart") && ParseUInt64(mapValue["timesmart"], &temp)) {

ryanofsky commented at 6:20 pm on November 6, 2019:

In commit “refactor: Replace use of atoi64 in timesmart unserialize” (f4218c26bbbcfc74ae23db9f9a9294447a8e9a6a)

Note: Slightly stricter parsing is done here (no whitespace or trailing characters allowed) but this should be fine since string just comes from calling strprintf("%u"). Could potentially log or return an error though if parsing fails, since it would be an indication of corruption.

ryanofsky commented at 6:35 pm on November 6, 2019: member

Partial ACK for checked commits below: all except 3. There seems to be a possible bug in the ParseMoney commit and I think two ArgsManager commits should be dropped and followed up in a separate PR (they could use improved error handling and probably release notes).

Could reopen this PR, or open a new one with the same changes. I can follow up later with another PR later if this stays dormant.

f7f2472d16be78bf9c2a7c85e5d40b3b78ef214a util: Add error handling to sequence id in bitcoin-tx (1/17)
df211cb3f02e1eb05eb6289af1688afd96660ca7 refactor: Replace use of stoul in dbwrapper.cpp (2/17)
f47450e1dcbd4e88efd8e69924da9d20930b399c refactor: Replace use of atoi in CleanupBlockRevFiles (3/17)
5cdaf4ad65b578c86ee21f9b7a7a48473a2ca735 refactor: Replace ad-hoc validity check and atoi in rpcconsole (4/17)
812c7327cc3ecdb474f3c89de9fa52f2d03be0ce refactor: Replace use of snprintf with strprintf in dbwrapper_tests (5/17)
a8f7502d21d8bbe0934c0c8674cb262d2e913c33 refactor: Replace use of strtol in rest.cpp (6/17)
d78e7be42300c4a313ad94aae998b04c25881cba refactor: Replace use of atoi for bool parsing in system.cpp (7/17)
a8ded10262c16778aaa87db0d8ba407e55ea0100 refactor: Replace use of atoi in torcontrol (8/17)
6992e0d387d10c805938a3cd1ffb4242d941a183 refactor: Remove now-unused atoi from util (9/17)
e4aa1f77b16c248488cc88b661bed69168c0f9e5 refactor: Replace use of atoi64 in core_read (10/17)
b7315cb5114477ede6ebb4e4c2303537dcb647b4 refactor: Replace use of atoi64 in rpc/mining.cpp (11/17)
2fcdd169cfcb1c0fcfb9d8a31fd9459d39a2dfa8 refactor: Replace use of atoi64 in moneystr.cpp (12/17)
afb58090ba4f834aa40730512f2485c09f33f32e refactor: Replace use of atoi64 in ArgsManager::GetArg (13/17)
87a3f13dfeff735aadb1091183f5269c9ab1a118 refactor: Replace use of atoi64 in wallet ReadOrderPos (14/17)
f4218c26bbbcfc74ae23db9f9a9294447a8e9a6a refactor: Replace use of atoi64 in timesmart unserialize (15/17)
88e83075406493304c3e33c9c1b6a451322ce634 refactor: Remove no-longer-used atoi64 from util (16/17)
55ba3887d00eec0eb27eee7e0499c6df9133d160 f: add TODO about returning parse error (17/17)

laanwj reopened this on Nov 6, 2019

laanwj commented at 6:55 pm on November 6, 2019: member

Thanks for the review, have reopened and will address your comments.

sidhujag referenced this in commit 9c45140570 on Nov 7, 2019

laanwj commented at 10:32 am on November 7, 2019: member

Sorry for messing around with this again, but I realized something. Before doing this it is important to have our own, controlled, implementation of Parse[U]Int(32|64) that does not call out to libc’s locale-dependent functions strtol/strtoll (these might still accept other kinds of numbers according to locale, for example). Starting here is like building a house on a foundation of jelly.

Adding WIP tag (it’s not possible to change a PR to draft, unfortunately).

laanwj renamed this:
~~refactor: Use our own integer parsing/formatting everywhere~~
WIP: refactor: Use our own integer parsing/formatting everywhere
on Nov 7, 2019

ryanofsky commented at 4:18 pm on November 7, 2019: member

Before doing this it is important to have our own, controlled, implementation of Parse[U]Int(32|64) that does not call out to libc’s locale-dependent functions strtol/strtoll (these might still accept other kinds of numbers according to locale, for example).

Agree this would make the change safer and easier to reason about. I don’t think strictly speaking most of the commits actually need this to be correct, either because looser parsings won’t cause harm or because current code is already using the locale anyway. But if we can remove locale from these places, it would be a good thing.

DrahtBot commented at 11:28 pm on November 8, 2019: member

DrahtBot added the label Needs rebase on Nov 8, 2019

laanwj closed this on Jan 4, 2020

HashUnlimited referenced this in commit eb33139d57 on Apr 17, 2020

MarcoFalke added the label Up for grabs on May 7, 2020

jasonbcox referenced this in commit d811715021 on Nov 4, 2020

sidhujag referenced this in commit 7492f2f2c1 on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022

WIP: refactor: Use our own integer parsing/formatting everywhere #17385

Conflicts