-
Notifications
You must be signed in to change notification settings - Fork 2.5k
GetLocalAgnostic<T> to construct streams with std::locale::classic #17807
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
Conversation
fb82064
to
a5e4eff
Compare
833b2e4
to
1e00aa0
Compare
1e00aa0
to
09736fd
Compare
09736fd
to
3f7a785
Compare
3f7a785
to
c109ac0
Compare
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. |
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.
Thanks for the PR! Instead of requiring people to use the |
Sure, I tried (and failed..) the easy way. |
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.
Handled via #17894, closing. |
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_tables
end 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.