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 character range formatting #3863

Merged
merged 9 commits into from
Mar 7, 2024
Merged

Conversation

js324
Copy link
Contributor

@js324 js324 commented Feb 23, 2024

My attempt to fix #3857. Implemented the :s and :?s specifier for ranges of characters. Specifically for the debug case (:?s), the underlying writer for escaped chars included single quotes in the output, so I converted the range into a string first. Added tests as well.

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/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
template <typename R, typename FormatContext>
template <typename R, typename FormatContext, typename U = T,
enable_if_t<std::is_same<U, Char>::value, bool> = true>
auto format(R&& range, FormatContext& ctx) const -> decltype(ctx.out()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid duplication let's merge the two format overloads and either use if constexpr or move only the string-specific logic into a separate function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried if constexpr but its introduced in C++17, so tests would break. I pushed the changes to make the string logic a separate function, though I'm unsure if there's a way to cleanly handle the non-Char case, so any advice here would be appreciated.

@js324 js324 requested a review from vitaut March 3, 2024 01:09
include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
@js324
Copy link
Contributor Author

js324 commented Mar 4, 2024

@vitaut Thanks for the comments, pushed the latest changes

@js324 js324 requested a review from vitaut March 4, 2024 21:07
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 good, just one remaining (minor) comment.

include/fmt/ranges.h Outdated Show resolved Hide resolved
@vitaut vitaut merged commit 0861db5 into fmtlib:master Mar 7, 2024
41 checks passed
@vitaut
Copy link
Contributor

vitaut commented Mar 7, 2024

Merged, thank you!

happymonkey1 pushed a commit to happymonkey1/fmt that referenced this pull request Apr 7, 2024
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.

support C++23 character range formatting
2 participants