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 join for specific types (#1981) #2034

Closed
wants to merge 1 commit into from

Conversation

kamibo
Copy link
Contributor

@kamibo kamibo commented Nov 25, 2020

Try to apply the same rules for iterator value type (join) as for
regular types.

Try to convert iterator value type to a formattable one.
For instance if the join operation refers to an enum class iterator
(such as std::byte) then try to use the enum class underlying type.

Also try to use a fallback formatter if available.

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

std::declval<typename std::iterator_traits<It>::value_type>()));

template <typename It, typename Char>
using iterator_formatter = conditional_t<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is definitely not so nice.
Also TBH I'm not sure that using FMT_BUFFER_CONTEXT is correct here.

Try to apply the same rules for iterator value type (join) as for
regular types.

Try to convert iterator value type to a formattable one.
For instance if the join operation refers to an enum class iterator
(such as std::byte) then try to use the enum class underlying type.

Also try to use a fallback formatter if available.
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.

You shouldn't be changing the formatter specialization for arg_join. Instead it's enough to add a formatter specialization for byte using FORMAT_AS here: https://github.com/fmtlib/fmt/blob/master/include/fmt/format.h#L3521 and add a cast in the implementation of FORMAT_AS.

@kamibo
Copy link
Contributor Author

kamibo commented Nov 25, 2020

I agree that it would fix the std::byte issue.
However it would not fix "similar issues".
Don't you think it would be nice to treat range value type the same way as "simple type"?
Ok let's take the std::byte issue from #1981, this is what I've understood so far:

fmt::print("{}", std::byte(42));

This works because of the conversion done here

return map(static_cast<typename std::underlying_type<T>::type>(val));

However

std::vector<std::byte> vv;

fmt::print("{}", fmt::join(vv, ","));

This does not work because no conversion occurs.
Here is the type used

: formatter<typename std::iterator_traits<It>::value_type, Char> {

Am I missing something?

So I tried to apply the same conversion.
I agree that while it works, it is not satisfying but it solves "all the cases".

@vitaut
Copy link
Contributor

vitaut commented Nov 27, 2020

Conversion is just an implementation detail. The current way of detecting if type is formattable is to check has_formatter which in turn checks formatter specializations. I don't think it's necessary to change that. formatter specialization for byte is useful anyway and adding it makes byte work with join and any other adapter.

@vitaut vitaut changed the title Fix join for specifc types (#1981) Fix join for specific types (#1981) Nov 27, 2020
@kamibo
Copy link
Contributor Author

kamibo commented Nov 30, 2020

I'm sorry but I feel quiet confused.
The issue is beyond std::byte.
It does not work regarding conversion issue and also it is not work with any "fallback formatter" (ostream).
For instance, in the code I work on, we have this:

struct RouteWithEvents
{
   ...
};

inline std::ostream& operator<<(std::ostream& os, const RouteWithEvents& route_with_events)
{
  return os << fmt::format("RouteWithEvents(Route: {}: {})", route_with_events.route, route_with_events.events_on_route);
}

using RoutesWithEvents = std::vector<RouteWithEvents>;

inline std::ostream& operator<<(std::ostream& os, const RoutesWithEvents& routes_with_events)
{
  return os << fmt::format("RoutesWithEvents({})", fmt::join(routes_with_events, ", "));
} 

Unless I missed something, this just doesn't work since 6.0.0 version while it works nicely with the previous version (5.3.0).
Do you consider this is not an issue?

@vitaut
Copy link
Contributor

vitaut commented Dec 3, 2020

Do you consider this is not an issue?

I'm OK with adding support for fallback formatter to join but it should be done via has_fallback_formatter.

@vitaut
Copy link
Contributor

vitaut commented Dec 3, 2020

Closing this in favor of std::byte-specific change: 4a6eadb.

@vitaut vitaut closed this Dec 3, 2020
@kamibo
Copy link
Contributor Author

kamibo commented Dec 4, 2020

Ok Thanks for your answer @vitaut!

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