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

Supporting ? as a string presentation type #2674

Merged
merged 15 commits into from
Jan 30, 2022

Conversation

brevzin
Copy link
Contributor

@brevzin brevzin commented Dec 24, 2021

Formatting ranges prints strings escaped, this allows the user to do so directly outside of a range context (if so desired).

Most of this PR is simply moving the logic from fmt/ranges.h to fmt/core.h. The width-counting (to support like... "{:*^10?}") using a counting output iterator is... probably not the right way to do this, but I'm not sure what the right way is.

@brevzin
Copy link
Contributor Author

brevzin commented Dec 24, 2021

So the issue here (as far as I can tell) is that when I moved the implementation from ranges.h to format.h, now it's suddenly getting used for detail::char8_type too in some contexts. In C++20 (where I was building locally), that's char8_t, which is convertible to char, so calls like this:

out = format_to(out, "\\x{:02x}", escape.cp);

work. But in C++11 (like some of the CI), it doesn't, complaining about how you can't call that:

../include/fmt/format.h:1661:26: error: no matching function for call to ‘format_to(std::back_insert_iterator<fmt::v8::detail::buffer<fmt::v8::detail::char8_type> >&, const char [9], uint32_t&)’
 1661 |           out = format_to(out, "\\U{:08x}", escape.cp);
      |                 ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

because the overload checks to see if the iterator is an output_iterator for char, and it's not if char8_type is just some enum.

The just-make-it-work solution (that I just pushed) is to only do the escaping logic for specifically char. That works on C++11 too. But like... obviously not the Right Thing To Do.

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/format.h Show resolved Hide resolved
include/fmt/format.h Outdated Show resolved Hide resolved
include/fmt/format.h Outdated Show resolved Hide resolved
include/fmt/format.h Outdated Show resolved Hide resolved
include/fmt/format.h Outdated Show resolved Hide resolved
include/fmt/format.h Show resolved Hide resolved
@vitaut
Copy link
Contributor

vitaut commented Jan 20, 2022

@brevzin, do you still plan working on this?

@brevzin
Copy link
Contributor Author

brevzin commented Jan 20, 2022

@brevzin, do you still plan working on this?

I do intend on getting to it eventually, but if you (or someone else) wants to take over, I'm totally fine with that. As you can tell, I'm not really sure how to actually do some of these things :) (well, except for the formatting comments - I can format the formatting PR at least. much meta. such wow).

@vitaut
Copy link
Contributor

vitaut commented Jan 20, 2022

I'm OK leaving the switch to code points for later (i.e. not this PR) and will merge the PR if you address the other (minor) comments.

support/printable.py Outdated Show resolved Hide resolved
@vitaut vitaut merged commit a34a97c into fmtlib:master Jan 30, 2022
@vitaut
Copy link
Contributor

vitaut commented Jan 30, 2022

Thank you!

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.

2 participants