From c434eefe94c73978605cf4dfe1459d2f8ede0858 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Thu, 3 Dec 2020 13:07:17 -0800 Subject: [PATCH 1/3] Change File::DeleteDir return value Makes File::DeleteDir return true when attempting to delete a nonexistent path. The purpose of DeleteDir is to ensure the path doesn't exist after the call, which is better reflected by the new return value. Additionally, none of the current callers actually check the return value so this won't break any existing code. --- Source/Core/Common/FileUtil.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Source/Core/Common/FileUtil.cpp b/Source/Core/Common/FileUtil.cpp index 5e8868da21..6039a449fa 100644 --- a/Source/Core/Common/FileUtil.cpp +++ b/Source/Core/Common/FileUtil.cpp @@ -257,6 +257,13 @@ bool DeleteDir(const std::string& filename) { INFO_LOG_FMT(COMMON, "DeleteDir: directory {}", filename); + // Return true because we care about the directory not being there, not the actual delete. + if (!File::Exists(filename)) + { + WARN_LOG_FMT(COMMON, "DeleteDir: {} does not exist", filename); + return true; + } + // check if a directory if (!IsDirectory(filename)) { From 4a55511e18c7bb912d882684d591d110576deb47 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Thu, 3 Dec 2020 13:13:16 -0800 Subject: [PATCH 2/3] Add warning flags to File deletion functions Adds a flag to File::Delete and File::DeleteDir functions to control whether a console warning is emitted when the file or directory doesn't exist. The flag is optional and true by default to match current behavior. --- Source/Core/Common/FileUtil.cpp | 14 ++++++++++---- Source/Core/Common/FileUtil.h | 12 ++++++++++-- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/Source/Core/Common/FileUtil.cpp b/Source/Core/Common/FileUtil.cpp index 6039a449fa..0d41395209 100644 --- a/Source/Core/Common/FileUtil.cpp +++ b/Source/Core/Common/FileUtil.cpp @@ -135,7 +135,7 @@ bool IsFile(const std::string& path) // Deletes a given filename, return true on success // Doesn't supports deleting a directory -bool Delete(const std::string& filename) +bool Delete(const std::string& filename, IfAbsentBehavior behavior) { INFO_LOG_FMT(COMMON, "Delete: file {}", filename); @@ -154,7 +154,10 @@ bool Delete(const std::string& filename) // Return true because we care about the file not being there, not the actual delete. if (!file_info.Exists()) { - WARN_LOG_FMT(COMMON, "Delete: {} does not exist", filename); + if (behavior == IfAbsentBehavior::ConsoleWarning) + { + WARN_LOG_FMT(COMMON, "Delete: {} does not exist", filename); + } return true; } @@ -253,14 +256,17 @@ bool CreateFullPath(const std::string& fullPath) } // Deletes a directory filename, returns true on success -bool DeleteDir(const std::string& filename) +bool DeleteDir(const std::string& filename, IfAbsentBehavior behavior) { INFO_LOG_FMT(COMMON, "DeleteDir: directory {}", filename); // Return true because we care about the directory not being there, not the actual delete. if (!File::Exists(filename)) { - WARN_LOG_FMT(COMMON, "DeleteDir: {} does not exist", filename); + if (behavior == IfAbsentBehavior::ConsoleWarning) + { + WARN_LOG_FMT(COMMON, "DeleteDir: {} does not exist", filename); + } return true; } diff --git a/Source/Core/Common/FileUtil.h b/Source/Core/Common/FileUtil.h index e593a0541f..7e0b2e6990 100644 --- a/Source/Core/Common/FileUtil.h +++ b/Source/Core/Common/FileUtil.h @@ -136,12 +136,20 @@ bool CreateDir(const std::string& filename); // Creates the full path of fullPath returns true on success bool CreateFullPath(const std::string& fullPath); +enum class IfAbsentBehavior +{ + ConsoleWarning, + NoConsoleWarning +}; + // Deletes a given filename, return true on success // Doesn't supports deleting a directory -bool Delete(const std::string& filename); +bool Delete(const std::string& filename, + IfAbsentBehavior behavior = IfAbsentBehavior::ConsoleWarning); // Deletes a directory filename, returns true on success -bool DeleteDir(const std::string& filename); +bool DeleteDir(const std::string& filename, + IfAbsentBehavior behavior = IfAbsentBehavior::ConsoleWarning); // renames file srcFilename to destFilename, returns true on success bool Rename(const std::string& srcFilename, const std::string& destFilename); From 760e7e664a38065c0bd1dbe978852ca85a48a160 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Thu, 3 Dec 2020 13:23:50 -0800 Subject: [PATCH 3/3] Add File::Delete and File::DeleteDir tests --- Source/UnitTests/FileUtil.cpp | 103 +++++++++++++++++++++++++++++ Source/UnitTests/UnitTests.vcxproj | 1 + 2 files changed, 104 insertions(+) create mode 100644 Source/UnitTests/FileUtil.cpp diff --git a/Source/UnitTests/FileUtil.cpp b/Source/UnitTests/FileUtil.cpp new file mode 100644 index 0000000000..bc07b753d5 --- /dev/null +++ b/Source/UnitTests/FileUtil.cpp @@ -0,0 +1,103 @@ +// Copyright 2020 Dolphin Emulator Project +// Licensed under GPLv2+ +// Refer to the license.txt file included. + +#include +#include + +#include "Common/FileUtil.h" + +class FileUtilTest : public testing::Test +{ +protected: + FileUtilTest() + : m_parent_directory(File::CreateTempDir()), m_file_path(m_parent_directory + "/file.txt"), + m_directory_path(m_parent_directory + "/dir"), + m_invalid_path(m_parent_directory + "/invalid.txt") + { + } + + ~FileUtilTest() override + { + if (!m_parent_directory.empty()) + { + File::DeleteDirRecursively(m_parent_directory); + } + } + + void SetUp() + { + if (m_parent_directory.empty()) + { + FAIL(); + } + } + + constexpr static std::array s_warning_behaviors = { + File::IfAbsentBehavior::ConsoleWarning, + File::IfAbsentBehavior::NoConsoleWarning, + }; + + const std::string m_parent_directory; + const std::string m_file_path; + const std::string m_directory_path; + const std::string m_invalid_path; +}; + +static void DeleteShouldNotRemoveDirectory(const std::string& path, File::IfAbsentBehavior behavior) +{ + File::CreateDir(path); + EXPECT_FALSE(File::Delete(path, behavior)); + File::DeleteDir(path, behavior); +} + +static void DeleteShouldRemoveFile(const std::string& path, File::IfAbsentBehavior behavior) +{ + File::CreateEmptyFile(path); + EXPECT_TRUE(File::Delete(path, behavior)); +} + +static void DeleteShouldReturnTrueForInvalidPath(const std::string& path, + File::IfAbsentBehavior behavior) +{ + EXPECT_TRUE(File::Delete(path, behavior)); +} + +static void DeleteDirShouldRemoveDirectory(const std::string& path, File::IfAbsentBehavior behavior) +{ + File::CreateDir(path); + EXPECT_TRUE(File::DeleteDir(path, behavior)); +} + +static void DeleteDirShouldNotRemoveFile(const std::string& path, File::IfAbsentBehavior behavior) +{ + File::CreateEmptyFile(path); + EXPECT_FALSE(File::DeleteDir(path, behavior)); + File::Delete(path, behavior); +} + +static void DeleteDirShouldReturnTrueForInvalidPath(const std::string& path, + File::IfAbsentBehavior behavior) +{ + EXPECT_TRUE(File::DeleteDir(path, behavior)); +} + +TEST_F(FileUtilTest, Delete) +{ + for (const auto behavior : s_warning_behaviors) + { + DeleteShouldNotRemoveDirectory(m_directory_path, behavior); + DeleteShouldRemoveFile(m_file_path, behavior); + DeleteShouldReturnTrueForInvalidPath(m_invalid_path, behavior); + } +} + +TEST_F(FileUtilTest, DeleteDir) +{ + for (const auto behavior : s_warning_behaviors) + { + DeleteDirShouldRemoveDirectory(m_directory_path, behavior); + DeleteDirShouldNotRemoveFile(m_file_path, behavior); + DeleteDirShouldReturnTrueForInvalidPath(m_invalid_path, behavior); + } +} diff --git a/Source/UnitTests/UnitTests.vcxproj b/Source/UnitTests/UnitTests.vcxproj index 3d3d0f4baf..d828e23389 100644 --- a/Source/UnitTests/UnitTests.vcxproj +++ b/Source/UnitTests/UnitTests.vcxproj @@ -68,6 +68,7 @@ +