Skip to content

Interpret precision as display width - #4443

Merged
vitaut merged 10 commits into
fmtlib:masterfrom
nikhilreddydev:master
May 25, 2025
Merged

Interpret precision as display width#4443
vitaut merged 10 commits into
fmtlib:masterfrom
nikhilreddydev:master

Conversation

@nikhilreddydev

Copy link
Copy Markdown
Contributor

Fixes #4272

It now correctly considers precision as display width precision.
Please let me know, if I need to add any test cases as well.

Thanks!

@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! Please address inline comments and add a test case to format-test.cc

Comment thread include/fmt/format.h Outdated
Comment thread include/fmt/format.h Outdated
@nikhilreddydev

nikhilreddydev commented May 22, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for the PR! Please address inline comments and add a test case to format-test.cc

Added the test case for introduced function dd25ea9.
Please review and happy to fix any further changes if any.
Sorry for the delay.

Comment thread test/format-test.cc Outdated
Comment on lines +214 to +216
EXPECT_EQ(
fmt::detail::count_code_points_with_display_width_precision("🐱🐱🐱", 5),
2);

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.

Let's test the output of fmt::format instead of this internal function to make sure that precision computation works end-to-end.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is done as part of 93fabd0

Thank you!

@vitaut
vitaut merged commit 6d79757 into fmtlib:master May 25, 2025
@vitaut

vitaut commented May 25, 2025

Copy link
Copy Markdown
Contributor

Merged, thanks!

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.

Use display width in precision

2 participants