Fix #875 - Unicode support: automatically widen on windows when option is assigned a std::filesystem::path #878

jmarrec · 2023-05-05T10:37:34Z

Fix Unicode support: automatically widen on windows when option is assigned a std::filesystem::path #875

Tested locally on Ubuntu 20.04 and Windows 10.

…RING=20 (or 17) to enable it

Testbed at https://godbolt.org/z/bb4j6d5qo

jmarrec · 2023-05-05T10:38:14Z

include/CLI/TypeTools.hpp

+// C++14
+template <class T> using remove_cv_t = typename std::remove_cv<T>::type;
+
+template <class T> using remove_reference_t = typename std::remove_reference<T>::type;
+
+// C++20
+template <class T> struct remove_cvref {
+    using type = remove_cv_t<remove_reference_t<T>>;
+};
+template <class T> using remove_cvref_t = typename remove_cvref<T>::type;


Backport some helpers to be in C++11 for readability

jmarrec · 2023-05-05T10:38:21Z

include/CLI/TypeTools.hpp

+#if defined CLI11_HAS_FILESYSTEM && CLI11_HAS_FILESYSTEM > 0
+#include <filesystem>
+#endif  // CLI11_HAS_FILESYSTEM


conditional import

jmarrec · 2023-05-05T10:39:48Z

include/CLI/TypeTools.hpp

+template <typename T, typename Enable = void> struct is_path : std::false_type {};
+template <typename T, typename Enable = void> struct force_widen : std::false_type {};
+
+#if defined CLI11_HAS_FILESYSTEM && CLI11_HAS_FILESYSTEM > 0
+template <typename T>
+struct is_path<T, typename std::enable_if<std::is_same<std::filesystem::path, remove_cvref_t<T>>::value>::type>
+    : std::true_type {};
+#if _WIN32
+template <typename T> struct force_widen<T, enable_if_t<is_path<T>::value>> : std::true_type {};
+#endif
+#else
+template <typename T> struct is_path : std::false_type {};
+#endif


Detect if T is a path, exists even if isn't used. I'm using remove_cvref, but that may not be necessary (but it doesn't hurt much)

if WIN32 => force_widen

I wrote it this way so force_widen could be extended to other types later if needed.

jmarrec · 2023-05-05T10:41:44Z

include/CLI/TypeTools.hpp

+struct classify_object<
+    T,
+    typename std::enable_if<!std::is_floating_point<T>::value && !std::is_integral<T>::value &&
+                            std::is_assignable<T &, std::string>::value && !force_widen<T>::value>::type> {


Here basically, I make it so that an object, in the event that it is both assignable and/or constructible from std::string AND from std::wstring, will be classified as wstring_assignable / wstring_constructible (the wide one) if it's force_widen

jmarrec · 2023-05-05T10:43:28Z

tests/CMakeLists.txt

@@ -69,7 +69,7 @@ set(CLI11_MULTIONLY_TESTS TimerTest)
 find_package(Catch2 CONFIG)

 if(Catch2_FOUND)
-  if(NOT TARGET Catch2::Catch2)
+  if(NOT (TARGET Catch2::Catch2 OR TARGET Catch2::Catch2WithMain))


Unrelated: I tried to build with Catch2 v3, hit this error. Then I hit another one and gave up (later it's trying to add_sources to catch_main whcih is just an ALIAS in the case it's v3).

jmarrec · 2023-05-05T10:43:39Z

tests/HelpersTest.cpp

+#if defined CLI11_HAS_FILESYSTEM && CLI11_HAS_FILESYSTEM > 0
+#include <filesystem>
+#endif  // CLI11_HAS_FILESYSTEM


Conditional import in test

jmarrec · 2023-05-05T10:44:23Z

tests/HelpersTest.cpp

+#if defined CLI11_HAS_FILESYSTEM && CLI11_HAS_FILESYSTEM > 0
+static_assert(!CLI::is_path<float>::value, "float shouldn't be a path");
+static_assert(!CLI::is_path<std::string>::value, "string shouldn't be a path");
+
+static_assert(CLI::is_path<std::filesystem::path>::value, "path should be a path");
+static_assert(CLI::is_path<const std::filesystem::path>::value, "const path should be a path");
+static_assert(CLI::is_path<std::filesystem::path &>::value, "path& should be a path");
+static_assert(CLI::is_path<const std::filesystem::path &>::value, "const path& should be a path");
+
+#if _WIN32
+static_assert(CLI::detail::classify_object<std::filesystem::path>::value ==
+                  CLI::detail::object_category::wstring_assignable,
+              "path is not classified as wstring_assignable on WIN32");
+static_assert(CLI::force_widen<std::filesystem::path>::value, "path should be force_widen on WIN32");
+static_assert(CLI::force_widen<const std::filesystem::path>::value, "const path should be force_widen on WIN32");
+static_assert(CLI::force_widen<std::filesystem::path &>::value, "path& should be force_widen on WIN32");
+static_assert(CLI::force_widen<const std::filesystem::path &>::value, "const path& should be force_widen on WIN32");
+#else
+static_assert(CLI::detail::classify_object<std::filesystem::path>::value ==
+                  CLI::detail::object_category::string_assignable,
+              "path is not classified as string_assignable");
+
+static_assert(!CLI::force_widen<std::filesystem::path>::value, "path should not be force_widen");
+static_assert(!CLI::force_widen<const std::filesystem::path>::value, "const path should not be force_widen");
+static_assert(!CLI::force_widen<std::filesystem::path &>::value, "path& should not be force_widen");
+static_assert(!CLI::force_widen<const std::filesystem::path &>::value, "const path& should not be force_widen");
+#endif
+
+TEST_CASE("Types: LexicalConversionPath", "[helpers]") {
+    CLI::results_t input = {"TéstFileNotUsed.txt"};
+    std::filesystem::path x;
+    bool res = CLI::detail::lexical_conversion<decltype(x), decltype(x)>(input, x);
+    CHECK(res);
+    CHECK(CLI::to_path("TéstFileNotUsed.txt").native() == x.native());
+}
+
+#endif  // CLI11_HAS_FILESYSTEM
+


conditional testing: lots of static_assert to make sure it's behaving correctly, and an actual runtime test that fails before, works now.

phlptp · 2023-06-15T13:29:11Z

this issue was resolved by #876, if there are unresolved issues from that please feel free to resubmit.
Thanks

jmarrec added 5 commits May 5, 2023 09:44

dd a test for CLIUtils#875, make sure to pass -DCMAKE_CXX_STANDARD:ST…

ca05b40

…RING=20 (or 17) to enable it

Potential fix for CLIUtils#875

36d4b92

CLIUtils#875 - Correct TMP to widen path

ea3955f

Testbed at https://godbolt.org/z/bb4j6d5qo

Fixup issues

2e0d901

Resolve bug in template resolution

a3cf628

jmarrec commented May 5, 2023

View reviewed changes

jmarrec mentioned this pull request May 5, 2023

Unicode support: automatically widen on windows when option is assigned a std::filesystem::path #875

Closed

phlptp mentioned this pull request Jun 15, 2023

prioritize wide strings over strings on MSVC #876

Merged

phlptp closed this Jun 15, 2023

r2d3 mentioned this pull request Aug 3, 2023

UTF8/Unicode config filename not supported on Windows #912

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix #875 - Unicode support: automatically widen on windows when option is assigned a std::filesystem::path #878

Fix #875 - Unicode support: automatically widen on windows when option is assigned a std::filesystem::path #878

Uh oh!

jmarrec commented May 5, 2023 •

edited

Loading

Uh oh!

jmarrec May 5, 2023

Uh oh!

jmarrec May 5, 2023

Uh oh!

jmarrec May 5, 2023

Uh oh!

jmarrec May 5, 2023

Uh oh!

jmarrec May 5, 2023

Uh oh!

jmarrec May 5, 2023

Uh oh!

jmarrec May 5, 2023

Uh oh!

jmarrec May 5, 2023

Uh oh!

phlptp commented Jun 15, 2023

Uh oh!

Uh oh!

Fix #875 - Unicode support: automatically widen on windows when option is assigned a std::filesystem::path #878

Fix #875 - Unicode support: automatically widen on windows when option is assigned a std::filesystem::path #878

Uh oh!

Conversation

jmarrec commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phlptp commented Jun 15, 2023

Uh oh!

Uh oh!

jmarrec commented May 5, 2023 •

edited

Loading