Skip to content

Support formatting of subseconds - #3115

Merged
vitaut merged 3 commits into
fmtlib:masterfrom
patrickroocks:subseconds
Oct 12, 2022
Merged

vitaut merged 3 commits into
fmtlib:masterfrom
patrickroocks:subseconds

Conversation

@patrickroocks

@patrickroocks patrickroocks commented Sep 26, 2022

Copy link
Copy Markdown
Contributor

The formatting symbol {%S} should also print the subseconds, e.g., fmt::format("{:%S}", std::chrono::microseconds(1234567)) should print "01.234567".

Didn't implement it for localized output as this localized output looks like black magic to me... may be we can omit it for the localized formatting for the first time?

@vitaut

vitaut commented Sep 28, 2022

Copy link
Copy Markdown
Contributor

Please rebase.

@patrickroocks

Copy link
Copy Markdown
Contributor Author

Rebased on master.

@vitaut vitaut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR.

Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread test/chrono-test.cc Outdated
@patrickroocks

patrickroocks commented Oct 3, 2022

Copy link
Copy Markdown
Contributor Author

I reworked everything again.

  • The work regarding subseconds was already done in the chrono_formatter for formatting durations. I pulled the code out of that and put in helper functions write_sub_sec and format_sec_fractional used in the tm_writer and the chrono_formatter.
  • I had to move around many helper functions to make them available for write_sub_sec and format_sec_fractional and implicitly for the tm_writer.
  • This implicitly supports arbitrary ratios for the Period.
  • I found a bug for printing durations with floating point types. The unit test
    EXPECT_EQ(fmt::format("{:%S}", std::chrono::duration<double>{1.5}), "01.500000");
    didn't succeed, instead "01" was given as output. I changed the number of digits to at least 6, if the Rep type is fractional and round(value) != value.

Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated

@vitaut vitaut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mostly looks good, just a few more minor comments.

Comment thread include/fmt/chrono.h Outdated
Comment thread include/fmt/chrono.h Outdated
@vitaut
vitaut merged commit 9254cfa into fmtlib:master Oct 12, 2022
@vitaut

vitaut commented Oct 12, 2022

Copy link
Copy Markdown
Contributor

Thank you!

@patrickroocks
patrickroocks deleted the subseconds branch October 13, 2022 06:08
@vitaut

vitaut commented Oct 16, 2022

Copy link
Copy Markdown
Contributor

Fuzzer discovered an issue with this change. The following code

#include <fmt/chrono.h>

int main() {
  fmt::format("{:%S}", std::chrono::duration<char, std::ratio<1, 100> >(0x80));
}

now results in an assertion failure:

    frame #7: 0x000000010000dddb a.out`fmt::v9::detail::assert_fail(char const*, int, char const*) + 59
    frame #8: 0x000000010000ac3e a.out`long long fmt::v9::detail::to_nonnegative_int<long long, long long, 0>(long long, long long) + 94
    frame #9: 0x000000010000a8dd a.out`void fmt::v9::detail::write_fractional_seconds<char, std::__1::back_insert_iterator<fmt::v9::basic_memory_buffer<char, 500ul, std::__1::allocator<char>>>, std::__1::chrono::duration<char, std::__1::ratio<1l, 100l>>>(std::__1::back_insert_iterator<fmt::v9::basic_memory_buffer<char, 500ul, std::__1::allocator<char>>>&, std::__1::chrono::duration<char, std::__1::ratio<1l, 100l>>) + 173

@phprus phprus mentioned this pull request Oct 16, 2022
@phprus

phprus commented Oct 16, 2022

Copy link
Copy Markdown
Contributor

@vitaut
Fix: #3143

@patrickroocks

Copy link
Copy Markdown
Contributor Author

Thanks for the fix and sorry that I have overseen this!

@vitaut

vitaut commented Oct 17, 2022

Copy link
Copy Markdown
Contributor

Here's a link to the oss-fuzz report for future reference: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=52380.

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.

3 participants