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

std::filesystem::path is not format as a range #3323

Closed
wants to merge 1 commit into from

Conversation

phprus
Copy link
Contributor

@phprus phprus commented Feb 26, 2023

Fix: #3322

@phprus phprus mentioned this pull request Feb 26, 2023
@vitaut
Copy link
Contributor

vitaut commented Feb 26, 2023

std::filesystem::path should be excluded by the recursive range check in

static constexpr auto value = std::is_same<range_reference_type<T>, T>::value

See e.g. http://eel.is/c++draft/format#range.fmtkind-2. I wonder why it doesn't work on Windows?

@phprus
Copy link
Contributor Author

phprus commented Feb 26, 2023

@vitaut

fmt/include/fmt/ranges.h

Lines 560 to 564 in 73b7cee

template <typename T, typename Char, typename Enable = void>
struct range_format_kind
: conditional_t<
is_range<T, Char>::value, detail::range_format_kind_<T>,
std::integral_constant<range_format, range_format::disabled>> {};

On windows std::filesystem::path uses wchar_t.
is_range exclude std::filesystem::path on unix because std::filesystem::path is convertible to std::basic_string<char>:

fmt/include/fmt/ranges.h

Lines 382 to 387 in 73b7cee

template <typename T, typename Char> struct is_range {
static constexpr const bool value =
detail::is_range_<T>::value && !detail::is_std_string_like<T>::value &&
!std::is_convertible<T, std::basic_string<Char>>::value &&
!std::is_convertible<T, detail::std_string_view<Char>>::value;
};

On Windows std::filesystem::path does not convertible to std::basic_string<char>.
Demo: https://godbolt.org/z/K1f1jPxjv

Original test for char (UNIX):

fmt/test/ranges-test.cc

Lines 182 to 191 in c644c75

struct path_like {
const path_like* begin() const;
const path_like* end() const;
operator std::string() const;
};
TEST(ranges_test, path_like) {
EXPECT_FALSE((fmt::is_range<path_like, char>::value));
}

@phprus
Copy link
Contributor Author

phprus commented Mar 1, 2023

@vitaut, what do you think about this PR?

@vitaut
Copy link
Contributor

vitaut commented Mar 4, 2023

I think it's OK for is_range to return true for path on Windows and the root cause is broken recursion check (it didn't strip cvref). This should be fixed in e0748e6. So I'm closing this PR but thanks anyway, especially for the test case which has been added in the above commit.

@vitaut vitaut closed this Mar 4, 2023
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.

Incompatibility of fmt/ranges.h and std::filesystem::path on Windows
2 participants