-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace remaining locale specific functions #2268
Comments
Note, if some of those functions cannot be replaced/updated, we should add them to the list of known violations to silence the linter. |
Where is the third sprintf use? I can only find 2 (and those are taken care of by #2269. |
It is 2, my mistake. Edited. |
The real issue with boost::to_lower is that it is a "modify in place" type call, where the bitcoin replacements return a value. |
strftime is used in DateTimeStrFormat, which is overloaded. It either takes a format string const char* pszFormat, or a default one is used. This means the format is specifically controlled and the specific use in DateTimeStrFormat can be added as an exception to the linter. The other direct uses also provide an output format string that is specific, so they can be added as exceptions too. |
Note that we have in strencodings.h... int atoi(const std::string& str) This takes the c_str() of the str which is fed into the atoi function. I don't believe this is locale specific. @denravonska can correct me if I am wrong. Given that this is really an overload of the built in atoi, and I don't think we can specify the argument types to distinguish, I think we should rename our atoi to strtoi so that we don't have to make an exception. Comments? I will go ahead and do a commit for that. |
Looks like I am wrong about that. the base atoi uses LC_NUMERIC of the current locale. |
Maybe I should replace all uses of atoi with ParseInt32, which is the wrapped version of strtol. What a pain. |
I implemented ParseInt and ParseUInt, modelled after the Parse(U)Int32 functions and replaced atoi with that. I can also go back and simplify a few other places I already did with these new functions. |
Ok. #2270 addresses everything remaining. |
^---- failure generated from test/lint/lint-include-guards.sh ^---- failure generated from test/lint/lint-includes.sh Unnecessary locale dependence can cause bugs that are very Advice not applicable in this specific case? Add an exception share/qt/extract_strings_qt.py:f = open(OUT_CPP, 'w') #2270 clears all of the locale entries except the strftime, for which we are going to put in exceptions in #2271. |
Does this mean the other lines in the Violation list can be cleared? Gridcoin-Research/test/lint/lint-locale-dependence.sh Lines 7 to 25 in fe6ab87
|
I think all can be cleared except |
#2271 is ready now |
The following locale specific functions remain after #2265 and #2266:
atoi(...)
- 6 uses (#2270 addresses)isxdigit(...)
- 1 use (#2270 addresses)printf(...)
- 3 uses (#2269 addresses)sprintf(...)
- 2 uses (see #2264 - #2269 addresses)stod(...)
- 4 uses (#2270 addresses)stof(...)
- 1 use (#2270 addresses)stoi(...)
- 8 uses (#2270 addresses - 2 of these are in leveldb, which should be excluded)stoul(...)
- 2 uses (#2270 addresses)stoull(...)
- 1 use (#2270 addresses)strftime(...)
- 3 uses (an exception should be put in to this - see comments below)strtod(...)
- 2 uses (#2270 addresses)strtold(...)
- 1 use (#2270 addresses)trim(...)
- 6 uses (#2270 addresses)Special case:
boost::to_lower
is triggering theto_lower
locale function check. Upstream replaced this boost call with a std implementation, but it requires a custom solution because the output from the boost call is not exactly the same. See bitcoin/bitcoin#13671 (#2270 addresses by replacing boost::to_lower(s) with s = ToLower(s).)The text was updated successfully, but these errors were encountered: