Skip to content

tests: Add std::to_string to list of locale dependent functions#17851

Merged
maflcko merged 1 commit intobitcoin:masterfrom
practicalswift:locale-dependence-of-to_string
Jan 3, 2020
Merged

tests: Add std::to_string to list of locale dependent functions#17851
maflcko merged 1 commit intobitcoin:masterfrom
practicalswift:locale-dependence-of-to_string

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jan 2, 2020

Add std::to_string to list of locale dependent functions:

std::to_string relies on the current locale for formatting purposes […]

Context #17808 (comment)

@fanquake fanquake added the Tests label Jan 2, 2020
@maflcko
Copy link
Member

maflcko commented Jan 2, 2020

ACK

hebasto
hebasto approved these changes Jan 2, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 1f0adb3, I have reviewed the code and it looks OK, I agree it can be merged.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 3, 2020

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #10443 (Add fee_est tool for debugging fee estimation code 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.

luke-jr
luke-jr suggested changes Jan 3, 2020
View reviewed changes
test/lint/lint-locale-dependence.sh
"src/qt/rpcconsole.cpp:.*atoi"
"src/rest.cpp:.*strtol"
"src/rpc/net.cpp.*std::to_string"
"src/rpc/rawtransaction.cpp.*std::to_string"
Copy link
Member

@luke-jr luke-jr Jan 3, 2020

Choose a reason for hiding this comment

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

This shouldn't be an exception, it's a bug?

Copy link
Contributor Author

@practicalswift practicalswift Jan 3, 2020

Choose a reason for hiding this comment

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

I'm not sure I follow: how is this a bug?

Do you mean the inclusion of src/rpc/rawtransaction.cpp in this list, or do you mean the usage of std::to_string in src/rpc/rawtransaction.cpp?

Please clarify :)

Applies also to the cases below.

Copy link
Member

@sipa sipa Jan 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I believe @luke-jr means that the current usage of that function in rpc/rawtransaction.cpp is a bug, and thus should not be silenced, but fixed.

Copy link
Member

@fanquake fanquake Jan 4, 2020

Choose a reason for hiding this comment

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

Opened an issue in #17866 so we don't lose track of this.

Copy link
Contributor Author

@practicalswift practicalswift Jan 4, 2020

Choose a reason for hiding this comment

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

@sipa Ah, got it. FWIW I consider all entries in KNOWN_VIOLATIONS to be potential bugs :)

test/lint/lint-locale-dependence.sh
"src/rest.cpp:.*strtol"
"src/rpc/net.cpp.*std::to_string"
"src/rpc/rawtransaction.cpp.*std::to_string"
"src/rpc/util.cpp.*std::to_string"
Copy link
Member

@luke-jr luke-jr Jan 3, 2020

Choose a reason for hiding this comment

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

I think this one is a bug too - we don't want RPC help locale-dependent, do we?

test/lint/lint-locale-dependence.sh
"src/rpc/net.cpp.*std::to_string"
"src/rpc/rawtransaction.cpp.*std::to_string"
"src/rpc/util.cpp.*std::to_string"
"src/test/addrman_tests.cpp.*std::to_string"
Copy link
Member

@luke-jr luke-jr Jan 3, 2020

Choose a reason for hiding this comment

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

Bug

test/lint/lint-locale-dependence.sh
"src/test/addrman_tests.cpp.*std::to_string"
"src/test/blockchain_tests.cpp.*std::to_string"
"src/test/dbwrapper_tests.cpp:.*snprintf"
"src/test/denialofservice_tests.cpp.*std::to_string"
Copy link
Member

@luke-jr luke-jr Jan 3, 2020

Choose a reason for hiding this comment

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

Bug? Or maybe we should be accepting locale numbers in args...

test/lint/lint-locale-dependence.sh
"src/test/denialofservice_tests.cpp.*std::to_string"
"src/test/fuzz/parse_numbers.cpp:.*atoi"
"src/test/key_tests.cpp.*std::to_string"
"src/test/net_tests.cpp.*std::to_string"
Copy link
Member

@luke-jr luke-jr Jan 3, 2020

Choose a reason for hiding this comment

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

bug

@maflcko
Copy link
Member

maflcko commented Jan 3, 2020

Going to push this doc/test-only change. Bug fixes can be solved in a separate pull request.

maflcko pushed a commit that referenced this pull request Jan 3, 2020
Merge #17851: tests: Add std::to_string to list of locale dependent f…
8164641 
…unctions

1f0adb3 tests: Add std::to_string to list of locale dependent functions (practicalswift)

Pull request description:

  Add `std::to_string` to list of locale dependent functions:

  > `std::to_string` relies on the current locale for formatting purposes […]

  Context #17808 (comment)

ACKs for top commit:
  hebasto:
    ACK 1f0adb3, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: 4cd6f567f5931dd166cdb9b065a939fb0bc02c93de18a9501655d98caf18b7c4d81f1881ea900dcdf2ec103d3ab1bdc9c68d3257b76dd2468a59e74d278b0d8d
@maflcko maflcko merged commit 1f0adb3 into bitcoin:master Jan 3, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 4, 2020
Merge bitcoin#17851: tests: Add std::to_string to list of locale depe…
c053415 
…ndent functions

1f0adb3 tests: Add std::to_string to list of locale dependent functions (practicalswift)

Pull request description:

  Add `std::to_string` to list of locale dependent functions:

  > `std::to_string` relies on the current locale for formatting purposes […]

  Context bitcoin#17808 (comment)

ACKs for top commit:
  hebasto:
    ACK 1f0adb3, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: 4cd6f567f5931dd166cdb9b065a939fb0bc02c93de18a9501655d98caf18b7c4d81f1881ea900dcdf2ec103d3ab1bdc9c68d3257b76dd2468a59e74d278b0d8d
@fanquake fanquake mentioned this pull request Jan 4, 2020
Closed
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
@ryanofsky ryanofsky mentioned this pull request Apr 27, 2020
Closed
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
Merge bitcoin#17851: tests: Add std::to_string to list of locale depe…
a15cf46 
…ndent functions

1f0adb3 tests: Add std::to_string to list of locale dependent functions (practicalswift)

Pull request description:

  Add `std::to_string` to list of locale dependent functions:

  > `std::to_string` relies on the current locale for formatting purposes […]

  Context bitcoin#17808 (comment)

ACKs for top commit:
  hebasto:
    ACK 1f0adb3, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: 4cd6f567f5931dd166cdb9b065a939fb0bc02c93de18a9501655d98caf18b7c4d81f1881ea900dcdf2ec103d3ab1bdc9c68d3257b76dd2468a59e74d278b0d8d
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 12, 2021
@Empact @PiRK
Replace std::to_string with locale-independent alternative
47a5b8a 
Summary:
> `std::to_string` relies on the current locale for formatting purposes

This is a backport of Core [[bitcoin/bitcoin#18134 | PR18134]] and [[bitcoin/bitcoin#17851 | PR17851]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien, majcosta

Reviewed By: #bitcoin_abc, Fabien, majcosta

Subscribers: majcosta, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8878
@practicalswift practicalswift deleted the locale-dependence-of-to_string branch April 10, 2021 19:39
kwvg added a commit to kwvg/dash that referenced this pull request Mar 24, 2022
@kwvg
merge bitcoin#17851: Add std::to_string to list of locale dependent f…
38fd597 
…unctions
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sipa sipa sipa left review comments

@fanquake fanquake fanquake left review comments

@hebasto hebasto hebasto approved these changes

+1 more reviewer

@luke-jr luke-jr luke-jr requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@practicalswift @maflcko @DrahtBot @sipa @fanquake @luke-jr @hebasto