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

Fix partial specialization problem for filesystem for Visual Studio #2957

Merged
merged 7 commits into from
Jul 3, 2022
Merged

Fix partial specialization problem for filesystem for Visual Studio #2957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5eeeb31
#2954: Provide std::conjunction and std::disjunction substitutes
Dani-Hub Jul 2, 2022
09f52a0
#2954: Use conjunction and disjunction substitute to make formatter s…
Dani-Hub Jul 2, 2022
8101036
#2954: As workaround for older MSVC compilers split formatter<std::fi…
Dani-Hub Jul 2, 2022
5e45aaa
2954: Add test case
Dani-Hub Jul 2, 2022
6eab289
Provide simplified implementations of conjunction and disjunction
Dani-Hub Jul 3, 2022
4cc8b6f
Remove workaround explicit specializations if the partial specializat…
Dani-Hub Jul 3, 2022
34e3b6f
Eliminate extra-test and merge it into existing std-test instead. Add…
Dani-Hub Jul 3, 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
18 changes: 18 additions & 0 deletions include/fmt/core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,24 @@ template <typename T> using type_identity_t = typename type_identity<T>::type;
template <typename T>
using underlying_t = typename std::underlying_type<T>::type;

template <typename...>
struct disjunction : std::false_type {};
template <typename P>
struct disjunction<P> : P {};
template <typename P1, typename... Pn>
struct disjunction<P1, Pn...>
: conditional_t<bool(P1::value), P1, disjunction<Pn...>> {
};

template <typename...>
struct conjunction : std::true_type {};
template <typename P>
struct conjunction<P> : P {};
template <typename P1, typename... Pn>
struct conjunction<P1, Pn...>
: conditional_t<bool(P1::value), conjunction<Pn...>, P1> {
};

struct monostate {
constexpr monostate() {}
};
Expand Down
41 changes: 25 additions & 16 deletions include/fmt/ranges.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ template <class Tuple, class F> void for_each(Tuple&& tup, F&& f) {
for_each(indexes, std::forward<Tuple>(tup), std::forward<F>(f));
}

#if FMT_MSC_VERSION
#if FMT_MSC_VERSION && FMT_MSC_VERSION < 1920
// Older MSVC doesn't get the reference type correctly for arrays.
template <typename R> struct range_reference_type_impl {
using type = decltype(*detail::range_begin(std::declval<R&>()));
Expand Down Expand Up @@ -396,15 +396,18 @@ template <typename R, typename Char>
struct formatter<
R, Char,
enable_if_t<
fmt::is_range<R, Char>::value
// Workaround a bug in MSVC 2019 and earlier.
#if !FMT_MSC_VERSION
&&
(is_formattable<detail::uncvref_type<detail::maybe_const_range<R>>,
Char>::value ||
detail::has_fallback_formatter<
detail::uncvref_type<detail::maybe_const_range<R>>, Char>::value)
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>
>
#endif
>::value
>> {

using range_type = detail::maybe_const_range<R>;
Expand Down Expand Up @@ -457,14 +460,20 @@ struct formatter<
template <typename T, typename Char>
struct formatter<
T, Char,
enable_if_t<detail::is_map<T>::value
// Workaround a bug in MSVC 2019 and earlier.
#if !FMT_MSC_VERSION
&& (is_formattable<detail::uncvref_first_type<T>, Char>::value ||
detail::has_fallback_formatter<detail::uncvref_first_type<T>, Char>::value)
&& (is_formattable<detail::uncvref_second_type<T>, Char>::value ||
detail::has_fallback_formatter<detail::uncvref_second_type<T>, Char>::value)
enable_if_t<conjunction<detail::is_map<T>
// Workaround a bug in MSVC 2017 and earlier.
#if !FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920
,
disjunction<
is_formattable<detail::uncvref_first_type<T>, Char>,
detail::has_fallback_formatter<detail::uncvref_first_type<T>, Char>
>,
disjunction<
is_formattable<detail::uncvref_second_type<T>, Char>,
detail::has_fallback_formatter<detail::uncvref_second_type<T>, Char>
>
#endif
>::value
>> {
template <typename ParseContext>
FMT_CONSTEXPR auto parse(ParseContext& ctx) -> decltype(ctx.begin()) {
Expand Down
5 changes: 5 additions & 0 deletions include/fmt/std.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ 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 @@ -69,6 +73,7 @@ struct formatter<std::filesystem::path, Char>
basic_string_view<Char>(quoted.data(), quoted.size()), ctx);
}
};
Copy link
Contributor

@vitaut vitaut Jul 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think the above conditional branch is needed because character types other than char and wchar_t won't work anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you writing about a generic implementation?
Why?
path::string<Char>() (and path formatter) supports char, wchar_t, char8_t, char16_t, and char32_t types.

Copy link
Contributor

@vitaut vitaut Jul 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, you are right. string<Char>() does the magic. In this case I suggest keeping just the generic branch and not introducing a workaround for broken msvc 2017. If anyone wants to format a path on a broken compiler version they can always use .string(). And 2019+ will work with the other workaround.

Copy link
Contributor Author

@Dani-Hub Dani-Hub Jul 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

At least according to std::format specification only char and wchar_t are supported for charT, that's the reason for my suggested workaround in this case. According to my experience, MSVC 2017 is still heavily in use. If you still prefer giving up 2017 support, may I suggest then a revised solution that still detects the problematic MSVC version and does not provide the partial specialization in this case? Regardless of that decision the corresponding test needs to get an extra conditional anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you still prefer giving up 2017 support, may I suggest then a revised solution that still detects the problematic MSVC version and does not provide the partial specialization in this case?

Yes, I think it's better to disable path support on an older broken compiler completely rather than enable it partially for some character types. Detecting the problematic MSVC version sounds reasonable but maybe move it to a separate PR and leave this one to fixing 2019+?

Copy link
Contributor Author

@Dani-Hub Dani-Hub Jul 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not suggesting to disable path support unconditionally. I'm suggesting to enable it conditionally for all cases that we know to work, so effectively not providing any specialization if FMT_MSC_VERSION && FMT_MSC_VERSION < 1920.
The same conditional will need to be added to the ranges-std test anyway, so why providing functionality that we decide to not test, because it does not work? Does that make sense to you?

Copy link
Contributor

@vitaut vitaut Jul 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

which value would it have to keep the partial specialization without conditional?

It will keep the implementation simple if I understood your question correctly. We generally try to avoid conditional compilation unless absolutely necessary and limiting it to test significantly reduces the scope.

Also:

it's better to disable path support on an older broken compiler completely rather than enable it partially for some character types

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's better to disable path support on an older broken compiler completely rather than enable it partially for some character types

My revised solution wasn't suggesting to enable it partially for some character types.

Copy link
Contributor

Choose a reason for hiding this comment

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

The revised solution seems OK to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

#endif
FMT_END_NAMESPACE
#endif

Expand Down
19 changes: 18 additions & 1 deletion test/std-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,18 @@
// For the license information refer to format.h.

#include "fmt/std.h"
#include "fmt/ranges.h"

#include <string>
#include <vector>

#include "gtest/gtest.h"

TEST(std_test, path) {
#ifdef __cpp_lib_filesystem
// 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)
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 @@ -31,6 +36,18 @@ TEST(std_test, path) {
#endif
}

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)
auto p = std::filesystem::path("foo/bar.txt");
auto c = std::vector<std::string>{"abc", "def"};
EXPECT_EQ(fmt::format("path={}, range={}", p, c),
"path=\"foo/bar.txt\", range=[\"abc\", \"def\"]");
#endif
}

TEST(std_test, thread_id) {
EXPECT_FALSE(fmt::format("{}", std::this_thread::get_id()).empty());
}
Expand Down