-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix formatting std::chrono::duration types to wide strings #1533
Conversation
Thanks for the PR! Looks good but could you add a test case checking that |
I have committed a wide version of the FormatDefault case, will that work? |
Hmm, seems to be failing with the 42µs case. I might need to look at this more.
Edit: Yep, it looks like I missed a couple things with |
I think might be a source file encoding issue, so I used \u00B5 instead.
After some trial and error, I believe the µ issue was from the encoding of the source file itself, so I specialized all of the get_units for wchar_t and in the wide version I used I also added |
This should hopefully fix compilation on VS <2019
if (isnan(val)) return write_nan(); | ||
auto locale = context.locale().template get<std::locale>(); | ||
auto& facet = std::use_facet<std::time_put<char_type>>(locale); | ||
std::basic_ostringstream<char_type> os; | ||
os.imbue(locale); | ||
facet.put(os, os, ' ', &time, format, format + std::strlen(format)); | ||
facet.put(os, os, ' ', &time, format, modifier); |
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.
Good idea.
if (const char* unit = get_units<Period>()) { | ||
string_view s(unit); |
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.
I suggest making unit
a string_view
and removing s
.
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.
I'll need to rework some things to do this properly since get_units
can return a nullptr
, which fmt's string_view
doesn't handle properly as far as I know. Since it passes the pointer to std::char_traits<T>::length()
which will dereference it without checking.
So it either needs to stay as-is, or I could revise get_units
to return string_view
s, and specifically construct a null-data 0-length string_view from there, which wouldn't invoke the length
call.
Personally I think it would be cleaner to leave this as-is.
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.
Let's leave as is this then.
Merged, thanks! |
Fix formatting std::chrono::duration types to wide strings (fmtlib#1533)
I was getting several compiler errors when trying to format a
std::chrono::duration
to a wide string, and found that various parts ofchrono.h
had a hard dependency on narrow chars. I tried to make it agnostic as possible, and it seems to work fine for me now, although I haven't tested it with anything besidesstd::chrono::duration<double, std::milli>
.If anything needs to be changed or fixed to get this merged just let me know.
I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.