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.

Sign up for GitHub

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

Jump to bottom

Replacing strftime with std::time_put #2550

Merged
merged 7 commits into from
Oct 30, 2021
Merged

Replacing strftime with std::time_put #2550

merged 7 commits into from
Oct 30, 2021

Conversation

phprus
Copy link
Contributor

@phprus phprus commented Oct 17, 2021 •
edited
Loading

Continuation of PR #2544.

Draft, because I haven't yet tested the PR in all my environments.

@vitaut, what do you think about using the built-in month and weekday names in the case of the std::locale::classic() locale (without std::time_put calls)?

@phprus phprus marked this pull request as ready for review October 17, 2021 18:35
toughengineer
toughengineer reviewed Oct 18, 2021
View reviewed changes
include/fmt/chrono.h Outdated Show resolved Hide resolved
@phprus phprus force-pushed the optimize-tm-formatting-4 branch from f8410da to 330f39d Compare October 18, 2021 13:35
@vitaut vitaut changed the title Replacing strftime to std::time_put Replacing strftime with std::time_put Oct 19, 2021
@vitaut
Copy link
Contributor

vitaut commented Oct 20, 2021

what do you think about using the built-in month and weekday names in the case of the std::locale::classic() locale (without std::time_put calls)?

We should definitely use built-in names in this case.

@phprus phprus marked this pull request as draft October 20, 2021 15:54
@phprus phprus force-pushed the optimize-tm-formatting-4 branch 2 times, most recently from 1d04758 to 623cc58 Compare October 20, 2021 19:40
@phprus
Copy link
Contributor Author

phprus commented Oct 20, 2021

Done, with workaround to strange C-locale std::put_time formats (%c, %r).

@phprus phprus marked this pull request as ready for review October 20, 2021 19:48
@phprus phprus mentioned this pull request Oct 22, 2021
Merged
1 task
@phprus phprus force-pushed the optimize-tm-formatting-4 branch 3 times, most recently from 6a89657 to 5cae10b Compare October 22, 2021 19:12
@phprus
Copy link
Contributor Author

phprus commented Oct 24, 2021

@vitaut I think the PR is ready for a review.
I no longer see potential errors and UB in the code due to invalid tm values.

vitaut
vitaut requested changes Oct 24, 2021
View reviewed changes
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.

include/fmt/chrono.h Outdated
char modifier) -> std::string {
auto&& os = std::ostringstream();
char modifier)
-> decltype(std::basic_ostringstream<Char>().str()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

decltype(std::basic_ostringstream<Char>().str()) -> std::basic_string<Char>

include/fmt/chrono.h Outdated Show resolved Hide resolved
include/fmt/chrono.h Outdated Show resolved Hide resolved
include/fmt/chrono.h Outdated Show resolved Hide resolved
include/fmt/chrono.h Outdated Show resolved Hide resolved
include/fmt/chrono.h Outdated Show resolved Hide resolved
include/fmt/chrono.h Outdated Show resolved Hide resolved
@vitaut
Copy link
Contributor

vitaut commented Oct 24, 2021

Also please move the overflow fix to a separate PR.

@phprus
Copy link
Contributor Author

phprus commented Oct 24, 2021

I moving fix to separate PR #2564.

I will rebase this PR after merging the new PR #2564 into master.

toughengineer
toughengineer reviewed Oct 26, 2021
View reviewed changes
include/fmt/chrono.h Outdated Show resolved Hide resolved
@phprus phprus mentioned this pull request Oct 27, 2021
Merged
@phprus phprus marked this pull request as draft October 27, 2021 20:34
@vitaut
Copy link
Contributor

vitaut commented Oct 27, 2021

Now that the overflow fix is merged (thanks for moving into a separate PR), please rebase this one.

@phprus phprus force-pushed the optimize-tm-formatting-4 branch from 5cae10b to 6e4b06d Compare October 28, 2021 16:28
@phprus
Copy link
Contributor Author

phprus commented Oct 28, 2021

@vitaut PR is updated. Please review.

@phprus phprus marked this pull request as ready for review October 28, 2021 16:33
vitaut
vitaut requested changes Oct 30, 2021
View reviewed changes
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.

Looks great, just minor comments left.

include/fmt/chrono.h Outdated Show resolved Hide resolved
include/fmt/chrono.h Outdated
Comment on lines 1593 to 1597
#ifdef _WIN32
on_us_date();
*out_++ = ' ';
on_iso_time();
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

The locale-independent and C locale format should be consistent among platforms so let's remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Corresponding tests for Windows are disabled (https://github.com/fmtlib/fmt/runs/4055476293?check_suite_focus=true)

include/fmt/chrono.h Outdated
Comment on lines 1738 to 1740
#ifdef _WIN32
on_iso_time();
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this as well.

@phprus phprus force-pushed the optimize-tm-formatting-4 branch 2 times, most recently from ea7af83 to cc20c47 Compare October 30, 2021 14:50
vitaut
vitaut reviewed Oct 30, 2021
View reviewed changes
test/chrono-test.cc Outdated
spec_list.push_back("%Y-%m-%d %H:%M:%S");
#ifndef _WIN32
// Disabled on Windows, because this formats is not consistent among platforms
Copy link
Contributor

Choose a reason for hiding this comment

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

this -> these and please add punctuation (.) at the end of the sentence.

@phprus phprus force-pushed the optimize-tm-formatting-4 branch from cc20c47 to a9df99f Compare October 30, 2021 15:03
@vitaut vitaut merged commit 1031eed into fmtlib:master Oct 30, 2021
@vitaut
Copy link
Contributor

vitaut commented Oct 30, 2021

Thank you!

PoetaKodu pushed a commit to pacc-repo/fmt that referenced this pull request Nov 11, 2021
@phprus @PoetaKodu
Replacing strftime with std::time_put (fmtlib#2550)
64fa436 
* Fix unicode test

* Add xchar support to chrono formatter

* Replace strftime with std::time_put

* Add std::locale support to std::tm formatter

* Use predefined names and formats for C-locale

* Performance improvement

* Make locale-independent and C locale formats consistent among platforms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toughengineer toughengineer toughengineer left review comments

@vitaut vitaut vitaut requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@phprus @vitaut @toughengineer