KEMBAR78
Support character range formatting by js324 · Pull Request #3863 · fmtlib/fmt · GitHub
Skip to content

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!

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
@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.

@vitaut vitaut merged commit 0861db5 into fmtlib:master Mar 7, 2024
@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