Fix partial specialization problem for filesystem for Visual Studio #2957

Dani-Hub · 2022-07-02T17:11:45Z

PULL request for issue #2954

phprus · 2022-07-02T17:31:02Z

@Dani-Hub
Please resolve merge conflicts.

…tter specializations for ranges and maps more robust (especially for Visual Studio compiler family)

…td::filesystem::path> partial template specialization into two explicit specialization.

vitaut

Thanks for the PR.

vitaut · 2022-07-03T13:45:23Z

include/fmt/core.h

+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 {
+};


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.

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.

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.

vitaut · 2022-07-03T13:45:33Z

test/CMakeLists.txt

@@ -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)


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.

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.

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.

vitaut · 2022-07-03T13:47:47Z

include/fmt/std.h

@@ -69,6 +70,33 @@ struct formatter<std::filesystem::path, Char>
        basic_string_view<Char>(quoted.data(), quoted.size()), ctx);
  }
 };


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

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.

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.

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.

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+?

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?

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

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.

The revised solution seems OK to me.

…ion would cause an ambiguity error

… conditionals for filesystem::path testing that does not run into the ambiguity problem.

Dani-Hub · 2022-07-03T16:33:33Z

Can someone explain to me the red cross that occurs now at my revised changes in core.h and std.h?

vitaut · 2022-07-03T18:07:00Z

Merged, thanks!

Dani-Hub · 2022-07-03T18:08:28Z

Thanks for the review and guidance!

Dani-Hub added 4 commits July 2, 2022 20:27

fmtlib#2954: Provide std::conjunction and std::disjunction substitutes

5eeeb31

fmtlib#2954: Use conjunction and disjunction substitute to make forma…

09f52a0

…tter specializations for ranges and maps more robust (especially for Visual Studio compiler family)

fmtlib#2954: As workaround for older MSVC compilers split formatter<s…

8101036

…td::filesystem::path> partial template specialization into two explicit specialization.

2954: Add test case

5e45aaa

Dani-Hub force-pushed the master branch from abbda20 to 5e45aaa Compare July 2, 2022 18:33

vitaut requested changes Jul 3, 2022

View reviewed changes

Dani-Hub added 3 commits July 3, 2022 18:22

Provide simplified implementations of conjunction and disjunction

6eab289

Remove workaround explicit specializations if the partial specializat…

4cc8b6f

…ion would cause an ambiguity error

Eliminate extra-test and merge it into existing std-test instead. Add…

34e3b6f

… conditionals for filesystem::path testing that does not run into the ambiguity problem.

vitaut approved these changes Jul 3, 2022

View reviewed changes

vitaut merged commit d2a2320 into fmtlib:master Jul 3, 2022

vitaut mentioned this pull request Jul 30, 2022

Improve CI on Windows, deprecate AppVeyor #3007

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix partial specialization problem for filesystem for Visual Studio #2957

Fix partial specialization problem for filesystem for Visual Studio #2957

Dani-Hub commented Jul 2, 2022

phprus commented Jul 2, 2022

vitaut left a comment

vitaut Jul 3, 2022 •

edited

Loading

Dani-Hub Jul 3, 2022

vitaut Jul 3, 2022

Dani-Hub Jul 3, 2022

vitaut Jul 3, 2022 •

edited

Loading

Dani-Hub Jul 3, 2022

vitaut Jul 3, 2022

Dani-Hub Jul 3, 2022

vitaut Jul 3, 2022 •

edited

Loading

phprus Jul 3, 2022

vitaut Jul 3, 2022 •

edited

Loading

Dani-Hub Jul 3, 2022 •

edited

Loading

vitaut Jul 3, 2022

Dani-Hub Jul 3, 2022 •

edited

Loading

vitaut Jul 3, 2022 •

edited

Loading

Dani-Hub Jul 3, 2022

vitaut Jul 3, 2022

Dani-Hub Jul 3, 2022

Dani-Hub commented Jul 3, 2022 •

edited

Loading

vitaut commented Jul 3, 2022

Dani-Hub commented Jul 3, 2022

Fix partial specialization problem for filesystem for Visual Studio #2957

Fix partial specialization problem for filesystem for Visual Studio #2957

Conversation

Dani-Hub commented Jul 2, 2022

phprus commented Jul 2, 2022

vitaut left a comment

Choose a reason for hiding this comment

vitaut Jul 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaut Jul 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaut Jul 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaut Jul 3, 2022 • edited Loading

Choose a reason for hiding this comment

Dani-Hub Jul 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dani-Hub Jul 3, 2022 • edited Loading

Choose a reason for hiding this comment

vitaut Jul 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dani-Hub commented Jul 3, 2022 • edited Loading

vitaut commented Jul 3, 2022

Dani-Hub commented Jul 3, 2022

vitaut Jul 3, 2022 •

edited

Loading

vitaut Jul 3, 2022 •

edited

Loading

vitaut Jul 3, 2022 •

edited

Loading

vitaut Jul 3, 2022 •

edited

Loading

Dani-Hub Jul 3, 2022 •

edited

Loading

Dani-Hub Jul 3, 2022 •

edited

Loading

vitaut Jul 3, 2022 •

edited

Loading

Dani-Hub commented Jul 3, 2022 •

edited

Loading