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 nested format specs for ranges. #2673

Merged
merged 8 commits into from
Jan 8, 2022

Conversation

brevzin
Copy link
Contributor

@brevzin brevzin commented Dec 24, 2021

Basic support for nested formatters for ranges from P2286. This does not add any top-level range format specifiers (pad/align/width/empty/delimiter), but does add the placeholder colon for them for the future.

Comment on lines 604 to 626
using context = buffer_context<Char>;
using mapper = detail::arg_mapper<context>;
using element = detail::uncvref_type<R>;

template <typename T,
FMT_ENABLE_IF(has_formatter<remove_cvref_t<T>, context>::value)>
static auto map(T&& value) -> T&& {
return (T &&) value;
}
template <typename T,
FMT_ENABLE_IF(!has_formatter<remove_cvref_t<T>, context>::value)>
static auto map(T&& value) -> decltype(mapper().map((T &&) value)) {
return mapper().map((T &&) value);
}

using formatter_type = conditional_t<
is_formattable<element, Char>::value,
formatter<remove_cvref_t<decltype(map(std::declval<element>()))>, Char>,
detail::fallback_formatter<element, Char>>;
formatter_type underlying_;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I more or less copied this from join, but I feel like there's probably a more direct way of getting the right formatter_type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not if we want to support implicit conversions. I suggest moving it to a new trait in detail and using in both places.

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! Overall looks good but please address the inline comments.

@@ -229,7 +229,7 @@ template <class Tuple, class F> void for_each(Tuple&& tup, F&& f) {
}

template <typename Range>
using value_type =
using uncvref_type =
Copy link
Contributor

Choose a reason for hiding this comment

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

I think value_type is more descriptive because this gives the range value type. The fact that cvref is removed is less important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't give the value_type though, it gives you the range's reference - with cvref stripped. A lot of the time, that is the value_type, but not always. For example: for vector<bool> the value_type is bool but this gives you vector<bool>::reference (which is what we would actually use to format). Or zipping two vector<int>s has a value_type of pair<int, int> but this gives you pair<int&, int&>.

I wanted to change it mainly because calling it value_type is misleading - we never use the range's value_type for anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK but could you clarify this in the comment then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
@brevzin
Copy link
Contributor Author

brevzin commented Jan 2, 2022

Uh, I'm failing CI because of... throwing exceptions? I don't understand.

include/fmt/ranges.h Outdated Show resolved Hide resolved
@brevzin brevzin force-pushed the fmt-ranges-underyling-specs branch from cdfd111 to 8b681f1 Compare January 2, 2022 22:43
@vitaut
Copy link
Contributor

vitaut commented Jan 3, 2022

Uh, I'm failing CI because of... throwing exceptions? I don't understand.

Not sure what you are referring to but the error is

'parse': is not a member of 'fmt::v8::detail::fallback_formatter<int [3][2],Char,void>

@brevzin
Copy link
Contributor Author

brevzin commented Jan 3, 2022

Uh, I'm failing CI because of... throwing exceptions? I don't understand.

Not sure what you are referring to but the error is

'parse': is not a member of 'fmt::v8::detail::fallback_formatter<int [3][2],Char,void>

@phprus pointed out that it was because I was doing throw format_error(...) instead of FMT_THROW(format_error(...)). Unrelated to the int[3][2] problem -- the exceptions one was causing half the CIs to fail.

This one I don't understand. Only fails on Windows too?

@vitaut
Copy link
Contributor

vitaut commented Jan 6, 2022

Looks like on MSVC the formatter specialization for int (&)[3][2] somehow becomes disabled with these changes (has_formatter returns false).

@brevzin
Copy link
Contributor Author

brevzin commented Jan 7, 2022

Looks like on MSVC the formatter specialization for int (&)[3][2] somehow becomes disabled with these changes (has_formatter returns false).

This seems like an MSVC bug. Here's an example:

#include <utility>

namespace fmt2 {
    template <typename T, size_t N>
    auto range_begin(T(&)[N]) -> T*;

    template <typename R>
    using iterator_t = decltype(range_begin(std::declval<R&>()));

    template <typename R>
    using reference_t = decltype(*std::declval<iterator_t<R>&>());
}

using R = int[3][2];

static_assert(
    std::is_same_v<
        fmt2::iterator_t<R>, int(*)[2]
        >,
    "!"
);

static_assert(
    std::is_same_v<
        fmt2::reference_t<R>, int(&)[2]
        >,
    "!"
);

The reference type of int[3][2] should be int(&)[2]. But MSVC v19.16 thinks it's int(*&&)[2]. Which is not a range. It's like they totally ignore the *. MSVC v19.21 still miscompiles, MSVC v19.22 gets this right.

If it's just arrays, I guess could add special handling for arrays for MSVC? This is probably the same bug that you were already working around in this space:

// Workaround a bug in MSVC 2019 and earlier.

@brevzin brevzin force-pushed the fmt-ranges-underyling-specs branch from 9b636f6 to 8a04ce1 Compare January 7, 2022 15:15
@brevzin
Copy link
Contributor Author

brevzin commented Jan 7, 2022

I got an email about checks failing, but it looks like all the checks passed? I don't even know.

@vitaut vitaut merged commit 6e0f139 into fmtlib:master Jan 8, 2022
@vitaut
Copy link
Contributor

vitaut commented Jan 8, 2022

Merged, thanks!

@vitaut
Copy link
Contributor

vitaut commented Jan 8, 2022

@brevzin could you by any chance add a section documenting this new feature (similarly to https://github.com/fmtlib/fmt/blob/master/doc/syntax.rst#chrono-format-specifications)?

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