GetLocalAgnostic<T> to construct streams with std::locale::classic#17807
Closed
carlopi wants to merge 1 commit intoduckdb:v1.3-ossivalisfrom
Closed
GetLocalAgnostic<T> to construct streams with std::locale::classic#17807carlopi wants to merge 1 commit intoduckdb:v1.3-ossivalisfrom
carlopi wants to merge 1 commit intoduckdb:v1.3-ossivalisfrom
Conversation
fb82064 to
a5e4eff
Compare
833b2e4 to
1e00aa0
Compare
1e00aa0 to
09736fd
Compare
09736fd to
3f7a785
Compare
3f7a785 to
c109ac0
Compare
Contributor
Author
|
I think this should be good to be reviewed, name is a bit long maybe, unsure if there are preferred alternative implementations. PR is not exaustive, there are still many potentially problematic usages in duckdb/duckdb or in external In any case PR solves some of the problems, so should be already worth having in if implementation makes sense. |
Mytherin
added a commit
that referenced
this pull request
Jun 6, 2025
Similar to #17807, this adds locale-independent handling for isspace. I have to say I don't remember exactly if I manage to reproduce a problem with this or just looked right to have. Feedback on the implementation welcome.
Collaborator
|
Thanks for the PR! Instead of requiring people to use the |
Contributor
Author
|
Sure, I tried (and failed..) the easy way. |
Mytherin
added a commit
that referenced
this pull request
Jun 12, 2025
Improved version of #17807, using infrastructure from #17866 to check `std::stringstream` are not used anymore at compile time. Original problem I had bumped in duckdb-wasm, where there would be some yet to be properly understood factor that would change the global static default locale to an unsupported one that would, via a SQL call to `duckdb_tables` end up calling a locale-influenced stringstream that would then throw. Testing can also be done on native applying the following diff, yet to be added as testing mode ```diff diff --git a/test/unittest.cpp b/test/unittest.cpp index 1c04f98..a70847a2a6 100644 --- a/test/unittest.cpp +++ b/test/unittest.cpp @@ -30,6 +30,8 @@ int main(int argc, char *argv[]) { duckdb::unique_ptr<FileSystem> fs = FileSystem::CreateLocal(); string test_directory = DUCKDB_ROOT_DIRECTORY; + std::locale::global(std::locale("ja_JP.SJIS")); + int new_argc = 0; auto new_argv = duckdb::unique_ptr<char *[]>(new char *[argc]); for (int i = 0; i < argc; i++) { ``` Also cleans up comment from previous PR review at #17866 (comment) Note that other streams might (and will) have the same problems, so it would be handy to expand this further, here taking it one step further.
Contributor
Author
|
Handled via #17894, closing. |
This file contains hidden or 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
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.
Unsure on the implementation choice, I think adding a function on top is somewhat cleaner, but that might be me, feedback welcome.
Problem I had bumped in duckdb-wasm, where there would be some yet to be properly understood factor that would change the global static default locale to an unsupported one that would, via a SQL call to
duckdb_tablesend up calling a locale-influenced stringstream that would then thow.I covered the places (together with a follow up PR on
isspace) that were needed to have the standard set of tests pass even injecting a random locale from the one I had available via the following diff:TODO, but I think they could be submitted as follow ups, are increasing both testing and the surface moved to locale-independent functions.