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 4 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
26 changes: 26 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,32 @@ 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;
template <>
struct disjunction<> : std::false_type {};
template <typename P>
struct disjunction<P> : P {};
template <typename P1, typename P2>
struct disjunction<P1, P2> : std::conditional<P1::value, P1, P2>::type {};
template <typename P1, typename P2, typename P3, typename... Pn>
struct disjunction<P1, P2, P3, Pn...>
: std::conditional<P1::value, P1, disjunction<P2, P3, Pn...>>::type {
};
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.

Why not use a simpler implementation like the one from cppreference?

template<class...> struct disjunction : std::false_type { };
template<class B1> struct disjunction<B1> : B1 { };
template<class B1, class... Bn>
struct disjunction<B1, Bn...> 
    : std::conditional_t<bool(B1::value), B1, disjunction<Bn...>>  { };

And the same for conjunction.

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 suggested the current implementation, because it is the same one that I committed for libstdc++ years ago and that worked for all known compilers regardless of completeness of variadic templates and minor rule changes in variadic templates. That being said, I guess the suggested alternative will work for all supported compilers. While adjusting this, we also can add a minor compile-conditional regarding __cpp_lib_logical_traits and take advantage of the STL variant in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

we also can add a minor compile-conditional regarding __cpp_lib_logical_traits and take advantage of the STL variant in this case.

I suggest keeping it simple and not introducing conditional compilation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.


template <typename...>
struct conjunction;
template <>
struct conjunction<> : std::true_type {};
template <typename P>
struct conjunction<P> : P {};
template <typename P1, typename P2>
struct conjunction<P1, P2> : std::conditional<P1::value, P2, P1>::type {};
template <typename P1, typename P2, typename P3, typename... Pn>
struct conjunction<P1, P2, P3, Pn...>
: std::conditional<P1::value, conjunction<P2, P3, Pn...>, P1>::type {
};

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
28 changes: 28 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,7 @@ inline void write_escaped_path<std::filesystem::path::value_type>(

} // namespace detail

#if !FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920
template <typename Char>
struct formatter<std::filesystem::path, Char>
: formatter<basic_string_view<Char>> {
Expand All @@ -69,6 +70,33 @@ 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

#else
// Workaround for MSVC 2017 and earlier.
template <>
struct formatter<std::filesystem::path, char>
: formatter<basic_string_view<char>> {
template <typename FormatContext>
auto format(const std::filesystem::path& p, FormatContext& ctx) const ->
typename FormatContext::iterator {
basic_memory_buffer<char> quoted;
detail::write_escaped_path(quoted, p);
return formatter<basic_string_view<char>>::format(
basic_string_view<char>(quoted.data(), quoted.size()), ctx);
}
};
template <>
struct formatter<std::filesystem::path, wchar_t>
: formatter<basic_string_view<wchar_t>> {
template <typename FormatContext>
auto format(const std::filesystem::path& p, FormatContext& ctx) const ->
typename FormatContext::iterator {
basic_memory_buffer<wchar_t> quoted;
detail::write_escaped_path(quoted, p);
return formatter<basic_string_view<wchar_t>>::format(
basic_string_view<wchar_t>(quoted.data(), quoted.size()), ctx);
}
};
#endif
FMT_END_NAMESPACE
#endif

Expand Down
2 changes: 2 additions & 0 deletions test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,15 @@ add_fmt_test(printf-test)
add_fmt_test(ranges-test ranges-odr-test.cc)
add_fmt_test(scan-test)
add_fmt_test(std-test)
add_fmt_test(ranges-std-test)
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.

Please merge this into std-test. Having a separate binary for a single test case is an overkill and including fmt/ranges.h from std-test is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you really sure that you want that? This means that your preexisting test does not actually test anymore that the inclusion of stl.h suffices to get the wanted effects, it only tests that the combined inclusion has these effects. I really recommend to keep the separate test translation unit, because this introduces exactly the errornous situation.

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 really sure that you want that?

Yes. Just make sure to keep fmt/std.h as the first include. I'm not concerned about other interactions between headers.

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.

try_compile(compile_result_unused
${CMAKE_CURRENT_BINARY_DIR}
SOURCES ${CMAKE_CURRENT_LIST_DIR}/detect-stdfs.cc
OUTPUT_VARIABLE RAWOUTPUT)
string(REGEX REPLACE ".*libfound \"([^\"]*)\".*" "\\1" STDLIBFS "${RAWOUTPUT}")
if (STDLIBFS)
target_link_libraries(std-test ${STDLIBFS})
target_link_libraries(ranges-std-test ${STDLIBFS})
endif ()
add_fmt_test(unicode-test HEADER_ONLY)
if (MSVC)
Expand Down
26 changes: 26 additions & 0 deletions test/ranges-std-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Formatting library for C++ - tests for ranges and std combination
//
// Copyright (c) 2012 - present, Victor Zverovich
// All rights reserved.
//
// For the license information refer to format.h.
//
// Copyright (c) 2022 - present, Dani-Hub (Daniel Kruegler)
// All rights reserved

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

#include <string>
#include <vector>

#include "gtest/gtest.h"

TEST(ranges_std_test, format_vector_path) {
#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),
"path=\"foo/bar.txt\", range=[\"abc\", \"def\"]");
#endif
}