-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
<format>
assumes strings are encoded in the active code page
#1834
Conversation
tests/std/tests/P0645R10_text_formatting_legacy_text_encoding/test.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that you cooked that up in a few days makes my imposter syndrome super happy
tests/std/tests/P0645R10_text_formatting_legacy_text_encoding/test.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me, although the hack to hook into format.cpp to change the codepage needs to change.
I do observe that the ucrt uses the _setmbcp
and _getmbcp
functions, which seem to set a global (not great).
No lie: this looked even jankier to me today than it did yesterday. I won't mourn its passing. |
Refactors all `format` functionality with knowledge of character encodings into a new class `_Fmt_codec`. `_Fmt_codec` internally caches the `_Cvtvec` structure that other functions previously passed around, and makes use of a new "pure C ABI function" `__std_get_cvt` to retrieve character conversion info for the active codepage. The ABI function reuses the `__std_code_page` and `__std_win_error` types from `<xfilesystem_abi.h>`. These could be extracted into a more general `__msvc_win_abi.hpp` header, but I think `<xfilesystem_abi.h>` is lightweight enough to just include directly for now. Drive-by: * BUG: `_Parse_precision` was not skipping the trailing `}` in a dynamic precision. * Improvement: `test_parse_helper` now verifies that the parse consumes the entire input when passed an expected length of `npos`.
Now that #1821 has merged, I've force-pushed and retargeted this to Apologies for the force-push! 😸 |
format
assumes strings are encoded in the active code page<format>
assumes strings are encoded in the active code page
... fix 2 bugs exposed by that coverage, and address review comments. Drive-by: Remove the `_FORMAT_CODEPAGE` override from the `formatting_utf8` test which is unnecessary since we compile it with `/utf-8`.
tests/std/tests/P0645R10_text_formatting_legacy_text_encoding/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/P0645R10_text_formatting_legacy_text_encoding/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/P0645R10_text_formatting_legacy_text_encoding/test.cpp
Outdated
Show resolved
Hide resolved
Thanks for fixing this bug! 😻 🎉 |
Refactors all
format
functionality with knowledge of character encodings into a new class_Fmt_codec
._Fmt_codec
internally caches the_Cvtvec
structure that other functions previously passed around, and makes use of a new "pure C ABI function"__std_get_cvt
to retrieve character conversion info for the active codepage.The ABI function reuses the
__std_code_page
and__std_win_error
types from<xfilesystem_abi.h>
. These could be extracted into a more general__msvc_win_abi.hpp
header, but I think<xfilesystem_abi.h>
is lightweight enough to just include directly for now.Drive-by:
_Parse_precision
was not skipping the trailing}
in a dynamic precision.test_parse_helper
now verifies that the parse consumes the entire input when passed an expected length ofnpos
.