Skip to content
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

Support formatting of time_point with utc_clock #3110

Merged
merged 1 commit into from
Sep 28, 2022

Conversation

patrickroocks
Copy link
Contributor

According to the discussion in #3098 in this PR a formatter for std::chrono::time_point<std::chrono::utc_clock> is added. Also added a small unit test. fmtlib must be compiled with C++20 or later to use this, as std::chrono::utc_clock is a C++20 feature.

@patrickroocks patrickroocks force-pushed the utc_clock branch 3 times, most recently from edb0104 to b512318 Compare September 23, 2022 07:21
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Overall looks good, just a few nits inline.

include/fmt/chrono.h Outdated Show resolved Hide resolved
include/fmt/chrono.h Show resolved Hide resolved
test/chrono-test.cc Outdated Show resolved Hide resolved
@patrickroocks
Copy link
Contributor Author

Thanks for the review! Pushed the fixes.

@vitaut
Copy link
Contributor

vitaut commented Sep 26, 2022

Looks good but there are some CI failures.

@patrickroocks
Copy link
Contributor Author

The chrono test fails with

unknown file: error: C++ exception with description "The specified module could not be found." thrown in the test body.
(https://github.com/fmtlib/fmt/actions/runs/3125553362/jobs/5078549521#step:6:134)

As this happens only for MSC version 1929 but not for version 1933 (the newest one), I check for this version now.

@mwinterb
Copy link
Contributor

It's probably not related to the MSVC version. Windows doesn't "historically" use the IANA time zone names, and so to support them for tzdb, Microsoft's STL uses ICU to handle that translation. icu.dll is only included in newer versions of Windows. Windows Server 2019 is based on Windows 1809, so it is too old to have just "icu.dll".

to_sys for utc_time uses tzdb to handle leap seconds.

@phprus
Copy link
Contributor

phprus commented Sep 27, 2022

@patrickroocks, @vitaut,
Maybe disable this test for Windows?
Like here:

fmt/test/chrono-test.cc

Lines 153 to 156 in 48f525d

// MSVC:
// minkernel\crts\ucrt\src\appcrt\time\wcsftime.cpp(971) : Assertion failed:
// timeptr->tm_year >= -1900 && timeptr->tm_year <= 8099
#ifndef _WIN32

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe disable this test for Windows?

Fine with me.

Also if @mwinterb is correct then the current MSVC check will only mask the issue and should be removed.

@patrickroocks
Copy link
Contributor Author

Removed the MSVC check, disabled the utc_clock test for windows. Squashed the commits.

@vitaut vitaut merged commit ad71961 into fmtlib:master Sep 28, 2022
@vitaut
Copy link
Contributor

vitaut commented Sep 28, 2022

Thank you!

@patrickroocks patrickroocks deleted the utc_clock branch September 29, 2022 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants