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 parameter type of formatter<char*> #3432

Merged
merged 1 commit into from
May 20, 2023

Conversation

timsong-cpp
Copy link
Contributor

Since this is macro expansion, const Type& expands to const Char*& instead of the desired Char* const&.

@vitaut
Copy link
Contributor

vitaut commented May 13, 2023

Thanks for the PR but this specialization should be there at all, removed in aeedac5.

@vitaut vitaut closed this May 13, 2023
@timsong-cpp
Copy link
Contributor Author

timsong-cpp commented May 13, 2023

this specialization should [not] be there at all

Hmm, are you sure? I think the result of this removal is that I can still format a char* but not a range of it (and std::formatter does provide this specialization).

Actually, never mind, a quick test suggests that it still works, interesting...

@timsong-cpp
Copy link
Contributor Author

OK, range_formatter works because it uses the mapper, but not fmt::join for either ranges or tuples. I'm not sure why this inconsistency (within fmt and also with std::formatter) is desirable.

@vitaut
Copy link
Contributor

vitaut commented May 14, 2023

The char* formatter is redundant. The only reason it has a formatter in the standard is because there is no other ways to represent formattability there (yet).

@timsong-cpp
Copy link
Contributor Author

If I have a my::Optional<T> whose formatter wants to support all the specifiers supported by T, how should I write it so that it works for my::Optional<char*>?

Also as mentioned, this breaks fmt::join for both ranges and tuples: https://gcc.godbolt.org/z/45q54oe4T

@brevzin
Copy link
Contributor

brevzin commented May 15, 2023

A simple wrapping example now breaks:

#include <fmt/format.h>

template <typename T>
struct wrapped {
    T t;
};

template <typename T>
struct fmt::formatter<wrapped<T>> {
    fmt::formatter<T> underlying;

    constexpr auto parse(auto& ctx) {
        return underlying.parse(ctx);
    }

    auto format(wrapped<T> const& w, auto& ctx) const {
        return underlying.format(w.t, ctx);
    }
};

auto f(wrapped<char*> w) -> std::string {
    return fmt::format("{}", w); // error: formatter<char*>::formatter() is deleted
}

@vitaut vitaut reopened this May 17, 2023
@vitaut
Copy link
Contributor

vitaut commented May 17, 2023

Good point, reverted in 616a493. Let's reopen this.

@vitaut vitaut merged commit 08ef0d0 into fmtlib:master May 20, 2023
@vitaut
Copy link
Contributor

vitaut commented May 20, 2023

merged, thanks

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.

3 participants