search

fmtlib / fmt

A modern formatting library
https://fmt.dev
Other
20.55k stars 2.46k forks source link

Added formattable concept #3974

Closed matt77hias closed 4 months ago

matt77hias commented 4 months ago

I used static_asserts without a message (C++17) for the concept (C++20) tests. Is there a reason why is_formattable does not have some checks for the usual suspects that are formattable?

I am not familiar with the CI test setup, but do we already have C++20 coverage?

phprus commented 4 months ago

@vitaut, Maybe makes sense to reintroduced tests for g++-11 and C++20 into CI? CI currently does not have tests for g++ and C++20.

It was removed in this commit:

https://github.com/fmtlib/fmt/commit/0b5287f8b7d6ab06ac112b7ec877f043e2eedf3e#diff-21364b2e6fae1f2875cee1ab3daefb0685403687eaf8bc32b5c6eacda351c9d3

vitaut commented 4 months ago

Is there a reason why is_formattable does not have some checks for the usual suspects that are formattable?

I don't think the tests need to be exhaustive. It's enough to cover important categories of formattable types. We don't need to test every type that has a user-defined formatter.

I am not familiar with the CI test setup, but do we already have C++20 coverage?

We do.

Maybe makes sense to reintroduced tests for g++-11 and C++20 into CI?

I removed g++-11 because its installation was broken but a PR to reintroduce it (or another version that supports C++20) would be welcome.

matt77hias commented 4 months ago

I don't think the tests need to be exhaustive. It's enough to cover important categories of formattable types. We don't need to test every type that has a user-defined formatter.

I agree, but I would have expected some static_assert(is_formattable<...) alongside all these static_assert(!is_formattable<...).