Move narrow() & escape_backslashes_for_json() into common code

charles-lunarg · charles-lunarg · commit 5a7a92eea5f4 · 2024-11-27T07:17:55.000-07:00
Because these functions are requied on windows for paths to behave correct, it is good
to put them in common code functions (like EnvVarWrapper) instead of requiring all
code to remember to call them. Having JsonWriter handle the path fixup means less
changes of double application occuring.
diff --git a/tests/framework/json_writer.h b/tests/framework/json_writer.h
@@ -79,13 +79,25 @@ struct JsonWriter {
     void AddKeyedString(std::string const& key, std::string const& value) {
         CommaAndNewLine();
         Indent();
-        output += "\"" + key + "\": \"" + value + "\"";
+        output += "\"" + key + "\": \"" + escape(value) + "\"";
     }
     void AddString(std::string const& value) {
         CommaAndNewLine();
         Indent();
-        output += "\"" + value + "\"";
+        output += "\"" + escape(value) + "\"";
     }
+#if defined(WIN32)
+    void AddKeyedString(std::string const& key, std::wstring const& value) {
+        CommaAndNewLine();
+        Indent();
+        output += "\"" + key + "\": \"" + escape(narrow(value)) + "\"";
+    }
+    void AddString(std::wstring const& value) {
+        CommaAndNewLine();
+        Indent();
+        output += "\"" + escape(narrow(value)) + "\"";
+    }
+#endif
 
     void AddKeyedBool(std::string const& key, bool value) {
         CommaAndNewLine();
@@ -98,6 +110,19 @@ struct JsonWriter {
         output += std::string(value ? "true" : "false");
     }
 
+    // Json doesn't allow `\` in strings, it must be escaped. Thus we have to convert '\\' to '\\\\' in strings
+    static std::string escape(std::string const& in_path) {
+        std::string out;
+        for (auto& c : in_path) {
+            if (c == '\\')
+                out += "\\\\";
+            else
+                out += c;
+        }
+        return out;
+    }
+    static std::string escape(std::filesystem::path const& in_path) { return escape(narrow(in_path.native())); }
+
    private:
     void CommaAndNewLine() {
         if (stack.size() > 0) {
diff --git a/tests/framework/test_environment.cpp b/tests/framework/test_environment.cpp
@@ -528,17 +528,17 @@ TestICD& FrameworkEnvironment::add_icd(TestICDDetails icd_details) noexcept {
                 break;
             case (ManifestDiscoveryType::env_var):
                 if (icd_details.is_dir) {
-                    env_var_vk_icd_filenames.add_to_list(narrow(folder->location()));
+                    env_var_vk_icd_filenames.add_to_list(folder->location());
                 } else {
-                    env_var_vk_icd_filenames.add_to_list(narrow(folder->location() / new_manifest_path));
+                    env_var_vk_icd_filenames.add_to_list(folder->location() / new_manifest_path);
                 }
                 platform_shim->add_known_path(folder->location());
                 break;
             case (ManifestDiscoveryType::add_env_var):
                 if (icd_details.is_dir) {
-                    add_env_var_vk_icd_filenames.add_to_list(narrow(folder->location()));
+                    add_env_var_vk_icd_filenames.add_to_list(folder->location());
                 } else {
-                    add_env_var_vk_icd_filenames.add_to_list(narrow(folder->location() / new_manifest_path));
+                    add_env_var_vk_icd_filenames.add_to_list(folder->location() / new_manifest_path);
                 }
                 platform_shim->add_known_path(folder->location());
                 break;
@@ -590,17 +590,17 @@ void FrameworkEnvironment::add_layer_impl(TestLayerDetails layer_details, Manife
             if (category == ManifestCategory::explicit_layer) {
                 fs_ptr = &get_folder(ManifestLocation::explicit_layer_env_var);
                 if (layer_details.is_dir) {
-                    env_var_vk_layer_paths.add_to_list(narrow(fs_ptr->location()));
+                    env_var_vk_layer_paths.add_to_list(fs_ptr->location());
                 } else {
-                    env_var_vk_layer_paths.add_to_list(narrow(fs_ptr->location() / layer_details.json_name));
+                    env_var_vk_layer_paths.add_to_list(fs_ptr->location() / layer_details.json_name);
                 }
             }
             if (category == ManifestCategory::implicit_layer) {
                 fs_ptr = &get_folder(ManifestLocation::implicit_layer_env_var);
                 if (layer_details.is_dir) {
-                    env_var_vk_implicit_layer_paths.add_to_list(narrow(fs_ptr->location()));
+                    env_var_vk_implicit_layer_paths.add_to_list(fs_ptr->location());
                 } else {
-                    env_var_vk_implicit_layer_paths.add_to_list(narrow(fs_ptr->location() / layer_details.json_name));
+                    env_var_vk_implicit_layer_paths.add_to_list(fs_ptr->location() / layer_details.json_name);
                 }
             }
             platform_shim->add_known_path(fs_ptr->location());
@@ -609,17 +609,17 @@ void FrameworkEnvironment::add_layer_impl(TestLayerDetails layer_details, Manife
             if (category == ManifestCategory::explicit_layer) {
                 fs_ptr = &get_folder(ManifestLocation::explicit_layer_add_env_var);
                 if (layer_details.is_dir) {
-                    add_env_var_vk_layer_paths.add_to_list(narrow(fs_ptr->location()));
+                    add_env_var_vk_layer_paths.add_to_list(fs_ptr->location());
                 } else {
-                    add_env_var_vk_layer_paths.add_to_list(narrow(fs_ptr->location() / layer_details.json_name));
+                    add_env_var_vk_layer_paths.add_to_list(fs_ptr->location() / layer_details.json_name);
                 }
             }
             if (category == ManifestCategory::implicit_layer) {
                 fs_ptr = &get_folder(ManifestLocation::implicit_layer_add_env_var);
                 if (layer_details.is_dir) {
-                    add_env_var_vk_implicit_layer_paths.add_to_list(narrow(fs_ptr->location()));
+                    add_env_var_vk_implicit_layer_paths.add_to_list(fs_ptr->location());
                 } else {
-                    add_env_var_vk_implicit_layer_paths.add_to_list(narrow(fs_ptr->location() / layer_details.json_name));
+                    add_env_var_vk_implicit_layer_paths.add_to_list(fs_ptr->location() / layer_details.json_name);
                 }
             }
             platform_shim->add_known_path(fs_ptr->location());
@@ -740,7 +740,7 @@ std::string get_loader_settings_file_contents(const LoaderSettings& loader_setti
             for (const auto& config : setting.layer_configurations) {
                 writer.StartObject();
                 writer.AddKeyedString("name", config.name);
-                writer.AddKeyedString("path", escape_backslashes_for_json(config.path));
+                writer.AddKeyedString("path", config.path.native());
                 writer.AddKeyedString("control", config.control);
                 writer.AddKeyedBool("treat_as_implicit_manifest", config.treat_as_implicit_manifest);
                 writer.EndObject();
diff --git a/tests/framework/test_util.cpp b/tests/framework/test_util.cpp
@@ -117,15 +117,15 @@ void print_vector_of_strings(JsonWriter& writer, const char* object_name, std::v
     if (strings.size() == 0) return;
     writer.StartKeyedArray(object_name);
     for (auto const& str : strings) {
-        writer.AddString(escape_backslashes_for_json(str));
+        writer.AddString(std::filesystem::path(str).native());
     }
     writer.EndArray();
 }
 void print_vector_of_strings(JsonWriter& writer, const char* object_name, std::vector<std::filesystem::path> const& paths) {
     if (paths.size() == 0) return;
     writer.StartKeyedArray(object_name);
     for (auto const& path : paths) {
-        writer.AddString(escape_backslashes_for_json(path));
+        writer.AddString(path.native());
     }
     writer.EndArray();
 }
@@ -137,7 +137,7 @@ std::string ManifestICD::get_manifest_str() const {
     writer.StartObject();
     writer.AddKeyedString("file_format_version", file_format_version.get_version_str());
     writer.StartKeyedObject("ICD");
-    writer.AddKeyedString("library_path", escape_backslashes_for_json(lib_path));
+    writer.AddKeyedString("library_path", lib_path.native());
     writer.AddKeyedString("api_version", version_to_string(api_version));
     writer.AddKeyedBool("is_portability_driver", is_portability_driver);
     if (!library_arch.empty()) writer.AddKeyedString("library_arch", library_arch);
@@ -159,7 +159,7 @@ void ManifestLayer::LayerDescription::get_manifest_str(JsonWriter& writer) const
     writer.AddKeyedString("name", name);
     writer.AddKeyedString("type", get_type_str(type));
     if (!lib_path.empty()) {
-        writer.AddKeyedString("library_path", escape_backslashes_for_json(lib_path));
+        writer.AddKeyedString("library_path", lib_path.native());
     }
     writer.AddKeyedString("api_version", version_to_string(api_version));
     writer.AddKeyedString("implementation_version", std::to_string(implementation_version));
@@ -217,20 +217,6 @@ std::string ManifestLayer::get_manifest_str() const {
     return writer.output;
 }
 
-// Json doesn't allow `\` in strings, it must be escaped. Thus we have to convert '\\' to '\\\\' in strings
-std::string escape_backslashes_for_json(std::string const& in_path) {
-    std::string out;
-    for (auto& c : in_path) {
-        if (c == '\\')
-            out += "\\\\";
-        else
-            out += c;
-    }
-    return out;
-}
-std::string escape_backslashes_for_json(std::filesystem::path const& in_path) {
-    return escape_backslashes_for_json(narrow(in_path.native()));
-}
 namespace fs {
 // internal implementation helper for per-platform creating & destroying folders
 int create_folder(std::filesystem::path const& path) {
diff --git a/tests/framework/test_util.h b/tests/framework/test_util.h
@@ -98,6 +98,19 @@
 #define FRAMEWORK_EXPORT
 #endif
 
+// Define it here so that json_writer.h has access to these functions
+#if defined(WIN32)
+// Convert an UTF-16 wstring to an UTF-8 string
+std::string narrow(const std::wstring& utf16);
+// Convert an UTF-8 string to an UTF-16 wstring
+std::wstring widen(const std::string& utf8);
+#else
+// Do nothing passthrough for the sake of Windows & UTF-16
+std::string narrow(const std::string& utf16);
+// Do nothing passthrough for the sake of Windows & UTF-16
+std::string widen(const std::string& utf8);
+#endif
+
 #include "json_writer.h"
 
 // get_env_var() - returns a std::string of `name`. if report_failure is true, then it will log to stderr that it didn't find the
@@ -145,6 +158,15 @@ struct EnvVarWrapper {
         cur_value += list_item;
         set_env_var();
     }
+#if defined(WIN32)
+    void add_to_list(std::wstring const& list_item) {
+        if (!cur_value.empty()) {
+            cur_value += OS_ENV_VAR_LIST_SEPARATOR;
+        }
+        cur_value += narrow(list_item);
+        set_env_var();
+    }
+#endif
     void remove_value() const { remove_env_var(); }
     const char* get() const { return name.c_str(); }
     const char* value() const { return cur_value.c_str(); }
@@ -176,8 +198,6 @@ void print_error_message(LSTATUS status, const char* function_name, std::string
 struct ManifestICD;    // forward declaration for FolderManager::write
 struct ManifestLayer;  // forward declaration for FolderManager::write
 
-std::string escape_backslashes_for_json(std::string const& in_path);
-std::string escape_backslashes_for_json(std::filesystem::path const& in_path);
 namespace fs {
 
 int create_folder(std::filesystem::path const& path);
@@ -221,18 +241,6 @@ class FolderManager {
 // size_dst - number of characters in the dst array
 inline void copy_string_to_char_array(std::string const& src, char* dst, size_t size_dst) { dst[src.copy(dst, size_dst - 1)] = 0; }
 
-#if defined(WIN32)
-// Convert an UTF-16 wstring to an UTF-8 string
-std::string narrow(const std::wstring& utf16);
-// Convert an UTF-8 string to an UTF-16 wstring
-std::wstring widen(const std::string& utf8);
-#else
-// Do nothing passthrough for the sake of Windows & UTF-16
-std::string narrow(const std::string& utf16);
-// Do nothing passthrough for the sake of Windows & UTF-16
-std::string widen(const std::string& utf8);
-#endif
-
 #if defined(WIN32)
 typedef HMODULE loader_platform_dl_handle;
 inline loader_platform_dl_handle loader_platform_open_library(const wchar_t* lib_path) {
@@ -993,7 +1001,7 @@ inline std::string test_platform_executable_path() {
     if (ret > buffer.size()) return NULL;
     buffer.resize(ret);
     buffer[ret] = '\0';
-    return buffer;
+    return narrow(std::filesystem::path(buffer).native());
 }
 
 #endif
diff --git a/tests/loader_layer_tests.cpp b/tests/loader_layer_tests.cpp
@@ -1149,7 +1149,7 @@ TEST(ImplicitLayers, DuplicateLayersInVK_ADD_IMPLICIT_LAYER_PATH) {
     auto& layer1 = env.get_test_layer(0);
     layer1.set_description("actually_layer_1");
     layer1.set_make_spurious_log_in_create_instance("actually_layer_1");
-    env.add_env_var_vk_implicit_layer_paths.add_to_list(narrow(env.get_folder(ManifestLocation::override_layer).location()));
+    env.add_env_var_vk_implicit_layer_paths.add_to_list(env.get_folder(ManifestLocation::override_layer).location());
 
     env.add_implicit_layer(TestLayerDetails(ManifestLayer{}.add_layer(ManifestLayer::LayerDescription{}
                                                                           .set_name(same_layer_name_1)
@@ -2460,7 +2460,7 @@ TEST(ExplicitLayers, DuplicateLayersInVK_LAYER_PATH) {
     auto& layer1 = env.get_test_layer(0);
     layer1.set_description("actually_layer_1");
     layer1.set_make_spurious_log_in_create_instance("actually_layer_1");
-    env.env_var_vk_layer_paths.add_to_list(narrow(env.get_folder(ManifestLocation::override_layer).location()));
+    env.env_var_vk_layer_paths.add_to_list(env.get_folder(ManifestLocation::override_layer).location());
 
     env.add_explicit_layer(TestLayerDetails(ManifestLayer{}.add_layer(ManifestLayer::LayerDescription{}
                                                                           .set_name(same_layer_name_1)
@@ -2534,7 +2534,7 @@ TEST(ExplicitLayers, DuplicateLayersInVK_ADD_LAYER_PATH) {
     auto& layer1 = env.get_test_layer(0);
     layer1.set_description("actually_layer_1");
     layer1.set_make_spurious_log_in_create_instance("actually_layer_1");
-    env.add_env_var_vk_layer_paths.add_to_list(narrow(env.get_folder(ManifestLocation::override_layer).location()));
+    env.add_env_var_vk_layer_paths.add_to_list(env.get_folder(ManifestLocation::override_layer).location());
 
     env.add_explicit_layer(TestLayerDetails(ManifestLayer{}.add_layer(ManifestLayer::LayerDescription{}
                                                                           .set_name(same_layer_name_1)
diff --git a/tests/loader_regression_tests.cpp b/tests/loader_regression_tests.cpp
@@ -3882,7 +3882,7 @@ TEST(DuplicateRegistryEntries, Drivers) {
 TEST(LibraryLoading, SystemLocations) {
     FrameworkEnvironment env{};
     EnvVarWrapper ld_library_path("LD_LIBRARY_PATH", env.get_folder(ManifestLocation::driver).location().string());
-    ld_library_path.add_to_list(narrow(env.get_folder(ManifestLocation::explicit_layer).location()));
+    ld_library_path.add_to_list(env.get_folder(ManifestLocation::explicit_layer).location());
 
     auto& driver = env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2).set_library_path_type(LibraryPathType::default_search_paths))
                        .add_physical_device({});
diff --git a/tests/loader_settings_tests.cpp b/tests/loader_settings_tests.cpp
@@ -243,7 +243,7 @@ TEST(SettingsFile, SupportsMultipleSettingsSimultaneously) {
     }
     env.debug_log.clear();
     // Set one set to contain the current executable path
-    env.loader_settings.app_specific_settings.at(0).add_app_key(escape_backslashes_for_json(test_platform_executable_path()));
+    env.loader_settings.app_specific_settings.at(0).add_app_key(test_platform_executable_path());
     env.update_loader_settings(env.loader_settings);
     {
         auto layer_props = env.GetLayerProperties(1);

Original file line number	Diff line number	Diff line change
`@@ -243,7 +243,7 @@ TEST(SettingsFile, SupportsMultipleSettingsSimultaneously) {`
`243`	`243`	`}`
`244`	`244`	`env.debug_log.clear();`
`245`	`245`	`// Set one set to contain the current executable path`
`246`		`- env.loader_settings.app_specific_settings.at(0).add_app_key(escape_backslashes_for_json(test_platform_executable_path()));`
	`246`	`+ env.loader_settings.app_specific_settings.at(0).add_app_key(test_platform_executable_path());`
`247`	`247`	`env.update_loader_settings(env.loader_settings);`
`248`	`248`	`{`
`249`	`249`	`auto layer_props = env.GetLayerProperties(1);`