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.

Sign up for GitHub

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

Exclude recursive ranges from the formatter specialization for ranges #2974

Merged
merged 9 commits into from
Jul 10, 2022
Merged

Exclude recursive ranges from the formatter specialization for ranges #2974

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d511485
2954: Add test case
Dani-Hub Jul 2, 2022
6d14574
Eliminate extra-test and merge it into existing std-test instead. Add…
Dani-Hub Jul 3, 2022
26a2844
#2968: Introduce additional compile-time predicate to detect recursiv…
Dani-Hub Jul 6, 2022
c6f372b
#2968: Eliminate preprocessor condition that enables the formatter sp…
Dani-Hub Jul 6, 2022
a1ec9fc
#2968: Eliminate preprocessor condition that enables the test for the…
Dani-Hub Jul 6, 2022
e4c2e72
Use own bool_constant, which is available for all C++ versions
Dani-Hub Jul 6, 2022
7b05da7
Reintroduce previous workaround but restrict to VS 2015 for now
Dani-Hub Jul 6, 2022
bfff7ed
Comma fix
Dani-Hub Jul 6, 2022
c03288b
- Rename is_not_recursive_range to is_nonrecursive_range and add comm…
Dani-Hub Jul 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 24 additions & 10 deletions include/fmt/ranges.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,22 +390,36 @@ using range_formatter_type = conditional_t<
template <typename R>
using maybe_const_range =
conditional_t<has_const_begin_end<R>::value, const R, R>;

template <typename R>
struct is_not_recursive_range : bool_constant<
Copy link
Contributor

Choose a reason for hiding this comment

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

is_not_recursive_range -> is_nonrecursive_range

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed as suggested.

!std::is_same<detail::uncvref_type<R>, R>::value> {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Contrary to the name of this trait it only checks recursiveness but not whether R is a range. I suggest adding is_range check here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, we want to prevent instantiation if the is_recursive predicate unless the is_range predicate has evaluated to true. See above. We could rename it to something different, such as is_not_recursive, if you prefer and add a comment that says that we assume here that is_range is evaluated to true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please add a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added, also relating to P2286.


template <typename R, typename Char>
struct is_formattable_delayed : is_formattable<
detail::uncvref_type<detail::maybe_const_range<R>>, Char> {
};

template <typename R, typename Char>
struct has_fallback_formatter_delayed : detail::has_fallback_formatter<
detail::uncvref_type<detail::maybe_const_range<R>>, Char> {
};
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these for? I'd suggest keeping them inlined as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole point of this P/R is to ensure that the individual parts of the condition are evaluated in short-circuit manner. In C++ without language concepts this is only possible by defining type predicates that get only evaluated if the corresponding predicate template class definition becomes instantiated. Due to std::conjunction and friends we therefore need individual class templates. The trailing delayed is intended to emphasize their job.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you clarify this in a comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest either adding a generic thing to delay instantiation, like so:

template <template <class...> class F, typename... Args> struct delayed : F<Args...> { };

to be used as:

disjunction<
    delayed<is_formattable, detail::uncvref_type<detail::maybe_const_range<R>>, Char>,
    delayed<detail::has_fallback_formatter, detail::uncvref_type<detail::maybe_const_range<R>>, Char>
    >

or, if we're going to manually do our own thing, do all of it together:

template <class T, class Char>
struct delayed_fallback_formattable
    : disjunction<
        is_formattable<T, Char>,
        detail::has_fallback_formatter<T, Char>>
{ };

to be used as:

conjunction<
    A,
    B,
    delayed_fallback_formattable<detail::uncvref_type<detail::maybe_const_range<R>>, Char>
    >

Put differently - instead of writing two bespoke delayed traits, write either one generic delayer or one bespoke delayed trait.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good suggestion but note that has_fallback_formatter will soon go away so I suggest keeping two bespoke traits. Keeping both will simplify removing has_fallback_formatter_delayed later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. Ok that makes sense then. When you say soon, how soon?

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 merged now has_fallback_formatter_delayed into is_formattable_delayed and got rid of the former.

Copy link
Contributor

Choose a reason for hiding this comment

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

how soon?

In the next major release about a year from now.


} // namespace detail

template <typename R, typename Char>
struct formatter<
R, Char,
enable_if_t<
conjunction<fmt::is_range<R, Char>
// Workaround a bug in MSVC 2017 and earlier.
#if !FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920
,
disjunction<
is_formattable<detail::uncvref_type<detail::maybe_const_range<R>>,
Char>,
detail::has_fallback_formatter<
detail::uncvref_type<detail::maybe_const_range<R>>, Char>
>
conjunction<fmt::is_range<R, Char>,
detail::is_not_recursive_range<R>
// Workaround a bug in MSVC 2015 and earlier.
#if !FMT_MSC_VERSION || FMT_MSC_VERSION > 1900
,
disjunction<
detail::is_formattable_delayed<R, Char>,
detail::has_fallback_formatter_delayed<R, Char>
>
#endif
>::value
>> {
Expand Down
5 changes: 0 additions & 5 deletions include/fmt/std.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,6 @@ inline void write_escaped_path<std::filesystem::path::value_type>(

} // namespace detail

#if !FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920
// For MSVC 2017 and earlier using the partial specialization
// would cause an ambiguity error, therefore we provide it only
// conditionally.
template <typename Char>
struct formatter<std::filesystem::path, Char>
: formatter<basic_string_view<Char>> {
Expand All @@ -73,7 +69,6 @@ struct formatter<std::filesystem::path, Char>
basic_string_view<Char>(quoted.data(), quoted.size()), ctx);
}
};
#endif
FMT_END_NAMESPACE
#endif

Expand Down
11 changes: 3 additions & 8 deletions test/std-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,7 @@
#include "gtest/gtest.h"

TEST(std_test, path) {
// Test ambiguity problem described in #2954. We need to exclude compilers
// where the ambiguity problem cannot be solved for now.
#if defined(__cpp_lib_filesystem) && \
(!FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920)
#ifdef __cpp_lib_filesystem
EXPECT_EQ(fmt::format("{:8}", std::filesystem::path("foo")), "\"foo\" ");
EXPECT_EQ(fmt::format("{}", std::filesystem::path("foo\"bar.txt")),
"\"foo\\\"bar.txt\"");
Expand All @@ -37,10 +34,8 @@ TEST(std_test, path) {
}

TEST(ranges_std_test, format_vector_path) {
// Test ambiguity problem described in #2954. We need to exclude compilers
// where the ambiguity problem cannot be solved for now.
#if defined(__cpp_lib_filesystem) && \
(!FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920)
// Test ambiguity problem described in #2954.
#ifdef __cpp_lib_filesystem
auto p = std::filesystem::path("foo/bar.txt");
auto c = std::vector<std::string>{"abc", "def"};
EXPECT_EQ(fmt::format("path={}, range={}", p, c),
Expand Down