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

Fix enable_ifs for map formatter: map formatter requires std::pair formatter but never uses it #2944

Merged
merged 1 commit into from
Jun 23, 2022

Conversation

BrukerJWD
Copy link
Contributor

I am referring to L455 in the formatter definition if is_map<T> is true:

fmt/include/fmt/ranges.h

Lines 449 to 456 in 5682338

template <typename T, typename Char>
struct formatter<
T, Char,
enable_if_t<detail::is_map<T>::value
// Workaround a bug in MSVC 2019 and earlier.
#if !FMT_MSC_VERSION
&& (is_formattable<detail::uncvref_type<T>, Char>::value ||
detail::has_fallback_formatter<detail::uncvref_type<T>,

Please note: T is the map type. We are using detail::uncvref_type<T> here, which gives the type of a dereferenced iterator of T, i.e. a std::pair<const K,V> in case of a std::map<K, V>.
This formatter is never used, though, as key and value are formatted separately:

fmt/include/fmt/ranges.h

Lines 476 to 479 in 5682338

out = detail::write_range_entry<Char>(out, item.first);
*out++ = ':';
*out++ = ' ';
out = detail::write_range_entry<Char>(out, item.second);

Thus, a formatter for the std::pair is not needed, or more generally speeking: L455 isn't checking the correct thing.

I stumbled upon this with a custom map which isn't using std::pair internally, and doesn't have a formatter for the internal pair type. But this shouldn't hinder this formatter from being used.
Additionally, the pair<K, V> formatter is no guarantee that we have formatters for K and V of the map (although it probably would be bad code not to use them in the pair formatter).

@vitaut vitaut merged commit e8bd2a8 into fmtlib:master Jun 23, 2022
@vitaut
Copy link
Contributor

vitaut commented Jun 23, 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