refactor: Remove locale-dependent function calls #156
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changes suggested in bitcoin/bitcoin#31741 (comment) to be able to enable more linters in Bitcoin Core CI. Co-authored-by: Ryan Ofsky <[email protected]>
ryanofsky
force-pushed
the
pr/locale
branch
2 times, most recently
from
6fb6897 to
c8351c5
Compare
|
Had to increase c++ version from c++17 to c++20 to use std::format Updated 38459f1 -> 6fb6897 ( |
std::format isn't available in g++12, or clang-16 either, so this could lead to issues later on? |
Thanks, this is good to know. Can revert std::format changes if needed later. They are definitely non-essential. |
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Feb 7, 2025
Bring in a few cmake changes to improve cmake support and fix compile/lint/tidy warnings. bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls
ryanofsky
added a commit
to ryanofsky/libmultiprocess
that referenced
this pull request
Feb 7, 2025
Partially revert bitcoin-core#156 due to lack of compiler support for std::format, as reported bitcoin-core#156 (comment) and bitcoin/bitcoin#31741 (comment) Technically it means this code is using locale dependent functions again, though I think in practice results will not be locale dependent because these are just integers not floating point numbers being formatted. This code could be written to use std::to_chars and be more independent, at the cost being slightly more complicated. Could be considered for a followup.
ryanofsky
added a commit
that referenced
this pull request
Feb 7, 2025
eca8fd3 refactor: Avoid using std::format (Ryan Ofsky) Pull request description: Partially revert #156 due to lack of compiler support for std::format, as reported #156 (comment) and bitcoin/bitcoin#31741 (comment) Technically it means this code is using locale dependent functions again, though I think in practice results will not be locale dependent because these are just integers not floating point numbers being formatted. This code could be written to use std::to_chars and be more independent, at the cost being slightly more complicated. Could be considered for a followup. Top commit has no ACKs. Tree-SHA512: f5178904232fb362aa113833d7080302cf448ba968a1daf9d8bad57320021d18a5247ea8959e0dd14d299fd3962f662e834bc4a302394507ede2023a3a7872d8
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Feb 7, 2025
Bring in a few cmake changes to improve cmake support and fix compile/lint/tidy warnings. bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format
hodlinator
reviewed
Feb 7, 2025
| char escape[4]; | ||
| snprintf(escape, 4, "\\%02x", c); | ||
| result.append(escape); | ||
| result.append(std::format("\\{:02x}", c)); |
There was a problem hiding this comment.
Not sure std::format works with current Bitcoin Core head:
https://github.com/bitcoin/bitcoin/blob/fb0ada982a73687520c43b8fde480fa5d456f3e1/doc/dependencies.md
Clang 16 < 17 which is required for the full "Texture Formatting" feature set according to https://en.cppreference.com/w/cpp/compiler_support/20#C.2B.2B20_library_features
Seems to check out on Godbolt: https://godbolt.org/z/9Ybd65f4d
There was a problem hiding this comment.
Oof, see now that maflcko beat me to it. :)
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Feb 10, 2025
Bring in a few cmake changes to improve cmake support and fix compile/lint/tidy warnings. bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done
Sjors
pushed a commit
to Sjors/bitcoin
that referenced
this pull request
Feb 10, 2025
Bring in a few cmake changes to improve cmake support and fix compile/lint/tidy warnings. bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project
Sjors
pushed a commit
to Sjors/bitcoin
that referenced
this pull request
Feb 13, 2025
Bring in a few cmake changes to improve cmake support and fix compile/lint/tidy warnings. bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Feb 24, 2025
Bump libmultiprocess library to include bugfix bitcoin-core/libmultiprocess#159 which should fix intermittent CI failure reported bitcoin#31921 This change is bumping the libmultiprocess version instead of cherry picking the bugfix. It could cherry-pick the bugfix instead, but there are reasons to prefer just bumping the version: - Bugfix might interact with earlier PRs, and the latest version is better tested with testing done in many CI configurations in bitcoin#30975 and bitcoin#31802 - Even though we are in feature freeze for a release, the MULTIPROCESS=1 option is currently not enabled for release, so this PR only affect CI builds and local builds, not the release build. bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project bitcoin-core/libmultiprocess#164 Bump minimum required cmake to 3.12
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Feb 24, 2025
Bump libmultiprocess library to include bugfix bitcoin-core/libmultiprocess#159 which should fix intermittent CI failures reported in bitcoin#31921 This change is bumping the libmultiprocess version instead of cherry picking the bugfix. It could cherry-pick the bugfix instead, but there are reasons to prefer bumping the version: - Bugfix might interact with earlier PRs, and the latest version is better tested with testing done in many CI configurations in bitcoin#30975 and bitcoin#31802 - Even though we are in feature freeze for a release, the MULTIPROCESS=1 option is currently not enabled for release, so this PR only affect CI builds and local builds, not the release build. This update brings in the following changes: bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project bitcoin-core/libmultiprocess#164 Bump minimum required cmake to 3.12
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Feb 24, 2025
Bump libmultiprocess library to include bugfix bitcoin-core/libmultiprocess#159 which should fix intermittent CI failures reported in bitcoin#31921 This change is bumping the libmultiprocess version instead of cherry picking the bugfix. It could cherry-pick the bugfix instead, but there are reasons to prefer bumping the version: - Bugfix might interact with earlier PRs, and the latest version is better tested with testing done in many CI configurations in bitcoin#30975 and bitcoin#31802 - Even though we are in feature freeze for a release, the MULTIPROCESS=1 option is currently not enabled for release, so this PR only affect CI builds and local builds, not the release build. This update brings in the following changes: bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project bitcoin-core/libmultiprocess#164 Bump minimum required cmake to 3.12
fanquake
pushed a commit
to bitcoin/bitcoin
that referenced
this pull request
Feb 25, 2025
Bump libmultiprocess library to include bugfix bitcoin-core/libmultiprocess#159 which should fix intermittent CI failures reported in #31921 This change is bumping the libmultiprocess version instead of cherry picking the bugfix. It could cherry-pick the bugfix instead, but there are reasons to prefer bumping the version: - Bugfix might interact with earlier PRs, and the latest version is better tested with testing done in many CI configurations in #30975 and #31802 - Even though we are in feature freeze for a release, the MULTIPROCESS=1 option is currently not enabled for release, so this PR only affect CI builds and local builds, not the release build. This update brings in the following changes: bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project bitcoin-core/libmultiprocess#164 Bump minimum required cmake to 3.12
fanquake
added a commit
to bitcoin/bitcoin
that referenced
this pull request
Feb 25, 2025
01f7715 depends: Update libmultiprocess library to fix CI failure (Ryan Ofsky) Pull request description: Bump libmultiprocess library to include bugfix bitcoin-core/libmultiprocess#159 which should fix intermittent CI failures reported in #31921 This change is bumping the libmultiprocess version instead of cherry picking the bugfix. It could cherry-pick the bugfix instead, but there are reasons to prefer bumping the version: - Bugfix might interact with earlier PRs, and the latest version is better tested with testing done in many CI configurations in [#30975](#30975) and [#31802](#31802) - Even though we are in feature freeze for a release, the MULTIPROCESS=1 option is currently not enabled for release, so this PR only affect CI builds and local builds, not the release build. This update brings in the following changes: bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project bitcoin-core/libmultiprocess#164 Bump minimum required cmake to 3.12 --- This PR is part of the [process separation project](#28722). ACKs for top commit: fanquake: ACK 01f7715 Tree-SHA512: a6a795e4d4e13e9d35c9346f3c83d5da817f1452bdc4a9412aeadc4c652ad6e5047f4c77756570594a70ec9095cc786772a0469e306dc19bb5a508fd515c37f4
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes suggested in bitcoin/bitcoin#31741 (comment) to be able to enable more linters in Bitcoin Core CI.