Skip to content

Replacing snprintf-based hex float formatter with internal implementation - #3179

Merged
vitaut merged 2 commits into
fmtlib:masterfrom
phprus:hexfloat-1
Nov 24, 2022
Merged

Replacing snprintf-based hex float formatter with internal implementation#3179
vitaut merged 2 commits into
fmtlib:masterfrom
phprus:hexfloat-1

Conversation

@phprus

@phprus phprus commented Nov 13, 2022

Copy link
Copy Markdown
Contributor
  1. Replacing snprintf-based hex float formatter with internal implementation.
  2. Add missing env CTEST_OUTPUT_ON_FAILURE.
  3. Enable C++17 tests on macOS for simplify hex float tests.

@phprus

phprus commented Nov 13, 2022

Copy link
Copy Markdown
Contributor Author

Benchmark:
https://github.com/phprus/fmt-bench/tree/hexfloat-1

MacbookPro M1 Max:

  • Default format string: without changes.
  • Compiled format string: ~3 times faster.

@phprus
phprus force-pushed the hexfloat-1 branch 3 times, most recently from 7bdac5c to 8dbd9fd Compare November 14, 2022 08:30
vitaut
vitaut requested changes Nov 16, 2022
View reviewed changes

@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! Overall looks great, a few minor comments inline. Also please move the build config changes into a separate PR.

Comment thread include/fmt/format.h Outdated
Comment thread include/fmt/format.h Outdated
Comment thread test/format-test.cc Outdated
Comment thread test/format-test.cc Outdated
@phprus phprus mentioned this pull request Nov 16, 2022
Merged
@phprus

phprus commented Nov 16, 2022

Copy link
Copy Markdown
Contributor Author

@vitaut CI PR: #3188
I will do rebase this PR after the merge PR #3188

@phprus
phprus marked this pull request as draft November 16, 2022 18:31
@phprus

phprus commented Nov 18, 2022

Copy link
Copy Markdown
Contributor Author

@vitaut, Rebased.

@phprus
phprus marked this pull request as ready for review November 18, 2022 16:50
@phprus
phprus requested a review from vitaut November 18, 2022 16:51
@phprus
phprus marked this pull request as draft November 23, 2022 16:20
@phprus
Add missing operators
071f471 
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
@phprus
Replace snprintf-based hex float formatter with internal implementation
b8f69a5 
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
@phprus
phprus marked this pull request as ready for review November 23, 2022 16:44
@phprus

phprus commented Nov 23, 2022

Copy link
Copy Markdown
Contributor Author

Rebased onto master

vitaut
vitaut reviewed Nov 24, 2022
View reviewed changes
Comment thread include/fmt/format.h

constexpr auto leading_shift = ((num_xdigits - 1) * 4);
const auto leading_mask = carrier_uint(0xF) << leading_shift;
const auto leading_v =

@vitaut vitaut Nov 24, 2022

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.

What does "v" stand for?

@phprus phprus Nov 24, 2022

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.

"value".

But, probably, it would be more correct name: "leading_[hex]digit" or "most_significant_[hex]digit".
Now I think leading_* are not good names.

Make a PR with renaming? If yes, which option should I choose?

@vitaut vitaut Nov 24, 2022

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.

A PR sounds good. I think leading_hexdigit or leading_xdigit (for consistency with num_xdigits) is fine. most_significant_* might be more precise but longer and I think leading is clear enough.

@phprus phprus Nov 24, 2022

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.

Done: PR #3203

@vitaut
vitaut merged commit 3136473 into fmtlib:master Nov 24, 2022
@vitaut

vitaut commented Nov 24, 2022

Copy link
Copy Markdown
Contributor

Merged, thanks!

@phprus phprus mentioned this pull request Nov 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vitaut vitaut vitaut approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@phprus @vitaut