refactor: fs::path-ify some util file functions

diff --git a/src/AtomicFile.hpp b/src/AtomicFile.hpp
index 033a3fa3c92eae326e849caee7e04277d1d9812a..89dd05902d11df537ccd361ee1a01524ac5facf7 100644 (file)
--- a/src/AtomicFile.hpp
+++ b/src/AtomicFile.hpp
@@ -1,4 +1,4 @@
-// Copyright (C) 2019-2022 Joel Rosdahl and other contributors
+// Copyright (C) 2019-2023 Joel Rosdahl and other contributors
 //
 // See doc/AUTHORS.adoc for a complete list of contributors.
 //
@@ -22,6 +22,7 @@
 
 #include <cstdint>
 #include <cstdio>
+#include <filesystem>
 #include <string>
 
 // This class represents a file whose data will be atomically written to a path
@@ -47,7 +48,7 @@ public:
 
 private:
   const std::string m_path;
-  std::string m_tmp_path;
+  std::filesystem::path m_tmp_path;
   FILE* m_stream;
 };
 

diff --git a/src/ccache.cpp b/src/ccache.cpp
index 378a0867c561ba0c544a5b3043c212e6d4640032..24511f99594659e67f97efb2587ed7421a315cec 100644 (file)
--- a/src/ccache.cpp
+++ b/src/ccache.cpp
@@ -708,7 +708,7 @@ result_key_from_depfile(Context& ctx, Hash& hash)
 struct GetTmpFdResult
 {
   Fd fd;
-  std::string path;
+  fs::path path;
 };
 
 static GetTmpFdResult
@@ -719,7 +719,7 @@ get_tmp_fd(Context& ctx,
   if (capture_output) {
     TemporaryFile tmp_stdout(
       FMT("{}/{}", ctx.config.temporary_dir(), description));
-    ctx.register_pending_tmp_file(tmp_stdout.path);
+    ctx.register_pending_tmp_file(tmp_stdout.path.string());
     return {std::move(tmp_stdout.fd), std::move(tmp_stdout.path)};
   } else {
     const auto dev_null_path = util::get_dev_null_path();
@@ -1232,7 +1232,7 @@ get_result_key_from_cpp(Context& ctx, Args& args, Hash& hash)
     // its thing correctly.
     TemporaryFile tmp_stdout(FMT("{}/cpp_stdout", ctx.config.temporary_dir()),
                              FMT(".{}", ctx.config.cpp_extension()));
-    preprocessed_path = tmp_stdout.path;
+    preprocessed_path = tmp_stdout.path.string();
     tmp_stdout.fd.close(); // We're only using the path.
     ctx.register_pending_tmp_file(preprocessed_path);
 

diff --git a/src/execute.cpp b/src/execute.cpp
index 8e04eddce5701404f9d2f22c6e1f0efc0716da21..78092a73c6470005e04e6d807c4bd4156d31df22 100644 (file)
--- a/src/execute.cpp
+++ b/src/execute.cpp
@@ -217,7 +217,7 @@ win32execute(const char* path,
 
   std::string args = Win32Util::argv_to_string(argv, sh);
   std::string full_path = Win32Util::add_exe_suffix(path);
-  std::string tmp_file_path;
+  fs::path tmp_file_path;
 
   Finalizer tmp_file_remover([&tmp_file_path] {
     if (!tmp_file_path.empty()) {

diff --git a/src/util/file.cpp b/src/util/file.cpp
index 107181c59f7946b95538726e2d51892cad2f7bcd..ec4037889f60f91c854cbd3cb6874cfab905383c 100644 (file)
--- a/src/util/file.cpp
+++ b/src/util/file.cpp
@@ -68,27 +68,25 @@ namespace fs = util::filesystem;
 namespace util {
 
 tl::expected<void, std::string>
-copy_file(const std::string& src,
-          const std::string& dest,
-          ViaTmpFile via_tmp_file)
+copy_file(const fs::path& src, const fs::path& dest, ViaTmpFile via_tmp_file)
 {
-  Fd src_fd(open(src.c_str(), O_RDONLY | O_BINARY));
+  Fd src_fd(open(src.string().c_str(), O_RDONLY | O_BINARY));
   if (!src_fd) {
     return tl::unexpected(
       FMT("Failed to open {} for reading: {}", src, strerror(errno)));
   }
 
-  unlink(dest.c_str());
+  unlink(dest.string().c_str());
 
   Fd dest_fd;
-  std::string tmp_file;
+  fs::path tmp_file;
   if (via_tmp_file == ViaTmpFile::yes) {
     TemporaryFile temp_file(dest);
     dest_fd = std::move(temp_file.fd);
     tmp_file = temp_file.path;
   } else {
-    dest_fd =
-      Fd(open(dest.c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666));
+    dest_fd = Fd(open(
+      dest.string().c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666));
     if (!dest_fd) {
       return tl::unexpected(
         FMT("Failed to open {} for writing: {}", dest, strerror(errno)));
@@ -115,7 +113,7 @@ copy_file(const std::string& src,
 }
 
 void
-create_cachedir_tag(const std::string& dir)
+create_cachedir_tag(const fs::path& dir)
 {
   constexpr char cachedir_tag[] =
     "Signature: 8a477f597d28d172789f06886806bc55\n"
@@ -123,8 +121,8 @@ create_cachedir_tag(const std::string& dir)
     "# For information about cache directory tags, see:\n"
     "#\thttp://www.brynosaurus.com/cachedir/\n";
 
-  const std::string path = FMT("{}/CACHEDIR.TAG", dir);
-  if (DirEntry(path).exists()) {
+  auto path = dir / "CACHEDIR.TAG";
+  if (fs::exists(path)) {
     return;
   }
   const auto result = write_file(path, cachedir_tag);
@@ -229,7 +227,7 @@ has_utf16_le_bom(std::string_view text)
 
 template<typename T>
 tl::expected<T, std::string>
-read_file(const std::string& path, size_t size_hint)
+read_file(const fs::path& path, size_t size_hint)
 {
   if (size_hint == 0) {
     DirEntry de(path);
@@ -249,7 +247,7 @@ read_file(const std::string& path, size_t size_hint)
       return O_RDONLY | O_BINARY;
     }
   }();
-  Fd fd(open(path.c_str(), open_flags));
+  Fd fd(open(path.string().c_str(), open_flags));
   if (!fd) {
     return tl::unexpected(strerror(errno));
   }
@@ -324,25 +322,25 @@ read_file(const std::string& path, size_t size_hint)
   return result;
 }
 
-template tl::expected<Bytes, std::string> read_file(const std::string& path,
+template tl::expected<Bytes, std::string> read_file(const fs::path& path,
                                                     size_t size_hint);
 
-template tl::expected<std::string, std::string>
-read_file(const std::string& path, size_t size_hint);
+template tl::expected<std::string, std::string> read_file(const fs::path& path,
+                                                          size_t size_hint);
 
 template tl::expected<std::vector<uint8_t>, std::string>
-read_file(const std::string& path, size_t size_hint);
+read_file(const fs::path& path, size_t size_hint);
 
 template<typename T>
 tl::expected<T, std::string>
-read_file_part(const std::string& path, size_t pos, size_t count)
+read_file_part(const fs::path& path, size_t pos, size_t count)
 {
   T result;
   if (count == 0) {
     return result;
   }
 
-  Fd fd(open(path.c_str(), O_RDONLY | O_BINARY));
+  Fd fd(open(path.string().c_str(), O_RDONLY | O_BINARY));
   if (!fd) {
     LOG("Failed to open {}: {}", path, strerror(errno));
     return tl::unexpected(strerror(errno));
@@ -380,16 +378,16 @@ read_file_part(const std::string& path, size_t pos, size_t count)
 }
 
 template tl::expected<Bytes, std::string>
-read_file_part(const std::string& path, size_t pos, size_t count);
+read_file_part(const fs::path& path, size_t pos, size_t count);
 
 template tl::expected<std::string, std::string>
-read_file_part(const std::string& path, size_t pos, size_t count);
+read_file_part(const fs::path& path, size_t pos, size_t count);
 
 template tl::expected<std::vector<uint8_t>, std::string>
-read_file_part(const std::string& path, size_t pos, size_t count);
+read_file_part(const fs::path& path, size_t pos, size_t count);
 
 tl::expected<bool, std::error_code>
-remove(const std::string& path, LogFailure log_failure)
+remove(const fs::path& path, LogFailure log_failure)
 {
   auto result = fs::remove(path);
   if (result || log_failure == LogFailure::yes) {
@@ -402,32 +400,33 @@ remove(const std::string& path, LogFailure log_failure)
 }
 
 tl::expected<bool, std::error_code>
-remove_nfs_safe(const std::string& path, LogFailure log_failure)
+remove_nfs_safe(const fs::path& path, LogFailure log_failure)
 {
   // fs::remove isn't atomic if path is on an NFS share, so we rename to a
   // temporary file. We don't care if the temporary file is trashed, so it's
   // always safe to remove it first.
-  std::string tmp_name =
-    FMT("{}.ccache{}remove", path, TemporaryFile::tmp_file_infix);
+  auto tmp_path =
+    path.parent_path()
+    / FMT("{}.ccache{}remove", path.filename(), TemporaryFile::tmp_file_infix);
 
-  auto rename_result = fs::rename(path, tmp_name);
+  auto rename_result = fs::rename(path, tmp_path);
   if (!rename_result) {
     // It's OK if it was removed in a race.
     if (rename_result.error().value() != ENOENT
         && rename_result.error().value() != ESTALE
         && log_failure == LogFailure::yes) {
-      LOG("Removing {} via {}", path, tmp_name);
+      LOG("Removing {} via {}", path, tmp_path);
       LOG("Renaming {} to {} failed: {}",
           path,
-          tmp_name,
+          tmp_path,
           rename_result.error().message());
     }
     return tl::unexpected(rename_result.error());
   }
 
-  auto remove_result = fs::remove(tmp_name);
+  auto remove_result = fs::remove(tmp_path);
   if (remove_result || log_failure == LogFailure::yes) {
-    LOG("Removing {} via {}", path, tmp_name);
+    LOG("Removing {} via {}", path, tmp_path);
     if (!remove_result) {
       LOG("Removal failed: {}", remove_result.error().message());
     }
@@ -436,7 +435,7 @@ remove_nfs_safe(const std::string& path, LogFailure log_failure)
 }
 
 void
-set_timestamps(const std::string& path,
+set_timestamps(const fs::path& path,
                std::optional<TimePoint> mtime,
                std::optional<TimePoint> atime)
 {
@@ -446,7 +445,7 @@ set_timestamps(const std::string& path,
     atime_mtime[0] = (atime ? *atime : *mtime).to_timespec();
     atime_mtime[1] = mtime->to_timespec();
   }
-  utimensat(AT_FDCWD, path.c_str(), mtime ? atime_mtime : nullptr, 0);
+  utimensat(AT_FDCWD, path.string().c_str(), mtime ? atime_mtime : nullptr, 0);
 #elif defined(HAVE_UTIMES)
   timeval atime_mtime[2];
   if (mtime) {
@@ -456,15 +455,15 @@ set_timestamps(const std::string& path,
     atime_mtime[1].tv_sec = mtime->sec();
     atime_mtime[1].tv_usec = mtime->nsec_decimal_part() / 1000;
   }
-  utimes(path.c_str(), mtime ? atime_mtime : nullptr);
+  utimes(path.string().c_str(), mtime ? atime_mtime : nullptr);
 #else
   utimbuf atime_mtime;
   if (mtime) {
     atime_mtime.actime = atime ? atime->sec() : mtime->sec();
     atime_mtime.modtime = mtime->sec();
-    utime(path.c_str(), &atime_mtime);
+    utime(path.string().c_str(), &atime_mtime);
   } else {
-    utime(path.c_str(), nullptr);
+    utime(path.string().c_str(), nullptr);
   }
 #endif
 }
@@ -472,10 +471,10 @@ set_timestamps(const std::string& path,
 #ifdef HAVE_DIRENT_H
 
 tl::expected<void, std::string>
-traverse_directory(const std::string& directory,
+traverse_directory(const fs::path& directory,
                    const TraverseDirectoryVisitor& visitor)
 {
-  DIR* dir = opendir(directory.c_str());
+  DIR* dir = opendir(directory.string().c_str());
   if (!dir) {
     return tl::unexpected(
       FMT("Failed to traverse {}: {}", directory, strerror(errno)));
@@ -490,7 +489,7 @@ traverse_directory(const std::string& directory,
       continue;
     }
 
-    std::string entry_path = directory + "/" + entry->d_name;
+    auto path = directory / entry->d_name;
     bool is_dir;
 #  ifdef _DIRENT_HAVE_D_TYPE
     if (entry->d_type != DT_UNKNOWN) {
@@ -498,25 +497,24 @@ traverse_directory(const std::string& directory,
     } else
 #  endif
     {
-      DirEntry dir_entry(entry_path);
+      DirEntry dir_entry(path);
       if (!dir_entry) {
         if (dir_entry.error_number() == ENOENT
             || dir_entry.error_number() == ESTALE) {
           continue;
         }
-        return tl::unexpected(FMT("Failed to lstat {}: {}",
-                                  entry_path,
-                                  strerror(dir_entry.error_number())));
+        return tl::unexpected(FMT(
+          "Failed to lstat {}: {}", path, strerror(dir_entry.error_number())));
       }
       is_dir = dir_entry.is_directory();
     }
     if (is_dir) {
-      traverse_directory(entry_path, visitor);
+      traverse_directory(path, visitor);
     } else {
-      visitor(entry_path, false);
+      visitor(path.string(), false);
     }
   }
-  visitor(directory, true);
+  visitor(directory.string(), true);
 
   return {};
 }
@@ -524,7 +522,7 @@ traverse_directory(const std::string& directory,
 #else // If not available, use the C++17 std::filesystem implementation.
 
 tl::expected<void, std::string>
-traverse_directory(const std::string& directory,
+traverse_directory(const fs::path& directory,
                    const TraverseDirectoryVisitor& visitor)
 {
   // Note: Intentionally not using std::filesystem::recursive_directory_iterator
@@ -546,7 +544,7 @@ traverse_directory(const std::string& directory,
         visitor(entry.path().string(), entry.is_directory());
       }
     }
-    visitor(directory, true);
+    visitor(directory.string(), true);
   } catch (const std::filesystem::filesystem_error& e) {
     return tl::unexpected(e.what());
   }
@@ -575,12 +573,13 @@ write_fd(int fd, const void* data, size_t size)
 }
 
 tl::expected<void, std::string>
-write_file(const std::string& path, std::string_view data, InPlace in_place)
+write_file(const fs::path& path, std::string_view data, InPlace in_place)
 {
   if (in_place == InPlace::no) {
-    unlink(path.c_str());
+    unlink(path.string().c_str());
   }
-  Fd fd(open(path.c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_TEXT, 0666));
+  Fd fd(
+    open(path.string().c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_TEXT, 0666));
   if (!fd) {
     return tl::unexpected(strerror(errno));
   }
@@ -588,14 +587,15 @@ write_file(const std::string& path, std::string_view data, InPlace in_place)
 }
 
 tl::expected<void, std::string>
-write_file(const std::string& path,
+write_file(const fs::path& path,
            nonstd::span<const uint8_t> data,
            InPlace in_place)
 {
   if (in_place == InPlace::no) {
-    unlink(path.c_str());
+    unlink(path.string().c_str());
   }
-  Fd fd(open(path.c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666));
+  Fd fd(
+    open(path.string().c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666));
   if (!fd) {
     return tl::unexpected(strerror(errno));
   }

diff --git a/src/util/file.hpp b/src/util/file.hpp
index 50e3afb299c6dbd2a8ca3d58b57524d71d18300b..7fd25003b75e0c5a8d010a5e8367320ee34a333a 100644 (file)
--- a/src/util/file.hpp
+++ b/src/util/file.hpp
@@ -28,6 +28,7 @@
 #include <cstddef>
 #include <cstdint>
 #include <ctime>
+#include <filesystem>
 #include <functional>
 #include <optional>
 #include <string>
@@ -48,11 +49,11 @@ using TraverseDirectoryVisitor =
 // Copy a file from `src` to `dest`. If `via_tmp_file` is yes, `src` is copied
 // to a temporary file and then renamed to dest.
 tl::expected<void, std::string>
-copy_file(const std::string& src,
-          const std::string& dest,
+copy_file(const std::filesystem::path& src,
+          const std::filesystem::path& dest,
           ViaTmpFile via_tmp_file = ViaTmpFile::no);
 
-void create_cachedir_tag(const std::string& dir);
+void create_cachedir_tag(const std::filesystem::path& dir);
 
 // Extends file size of `fd` to at least `new_size` by calling posix_fallocate()
 // if supported, otherwise by writing zeros last to the file.
@@ -83,7 +84,7 @@ tl::expected<util::Bytes, std::string> read_fd(int fd);
 // If `size_hint` is not 0 then it is assumed that `path` has this size (this
 // saves system calls).
 template<typename T>
-tl::expected<T, std::string> read_file(const std::string& path,
+tl::expected<T, std::string> read_file(const std::filesystem::path& path,
                                        size_t size_hint = 0);
 
 // Return (at most) `count` bytes from `path` starting at position `pos`.
@@ -94,7 +95,7 @@ tl::expected<T, std::string> read_file(const std::string& path,
 // UTF-8.
 template<typename T>
 tl::expected<T, std::string>
-read_file_part(const std::string& path, size_t pos, size_t count);
+read_file_part(const std::filesystem::path& path, size_t pos, size_t count);
 
 // Remove `path` (non-directory), NFS hazardous. Use only for files that will
 // not exist on other systems.
@@ -102,14 +103,15 @@ read_file_part(const std::string& path, size_t pos, size_t count);
 // Returns whether the file was removed. A nonexistent `path` is considered
 // successful.
 tl::expected<bool, std::error_code>
-remove(const std::string& path, LogFailure log_failure = LogFailure::yes);
+remove(const std::filesystem::path& path,
+       LogFailure log_failure = LogFailure::yes);
 
 // Remove `path` (non-directory), NFS safe.
 //
 // Returns whether the file was removed. A nonexistent `path` is considered a
 // successful.
 tl::expected<bool, std::error_code>
-remove_nfs_safe(const std::string& path,
+remove_nfs_safe(const std::filesystem::path& path,
                 LogFailure log_failure = LogFailure::yes);
 
 // Set the FD_CLOEXEC on file descriptor `fd`. This is a NOP on Windows.
@@ -117,14 +119,14 @@ void set_cloexec_flag(int fd);
 
 // Set atime/mtime of `path`. If `mtime` is std::nullopt, set to the current
 // time. If `atime` is std::nullopt, set to what `mtime` specifies.
-void set_timestamps(const std::string& path,
+void set_timestamps(const std::filesystem::path& path,
                     std::optional<TimePoint> mtime = std::nullopt,
                     std::optional<TimePoint> atime = std::nullopt);
 
 // Traverse `path` recursively in postorder (directory entries are visited
 // before their parent directory).
 tl::expected<void, std::string>
-traverse_directory(const std::string& directory,
+traverse_directory(const std::filesystem::path& directory,
                    const TraverseDirectoryVisitor& visitor);
 
 // Write `size` bytes from binary `data` to `fd`.
@@ -132,13 +134,13 @@ tl::expected<void, std::string> write_fd(int fd, const void* data, size_t size);
 
 // Write text `data` to `path`. If `in_place` is no, unlink any existing file
 // first (i.e., break hard links).
-tl::expected<void, std::string> write_file(const std::string& path,
+tl::expected<void, std::string> write_file(const std::filesystem::path& path,
                                            std::string_view data,
                                            InPlace in_place = InPlace::no);
 
 // Write binary `data` to `path`. If `in_place` is no, unlink any existing
 // file first (i.e., break hard links).
-tl::expected<void, std::string> write_file(const std::string& path,
+tl::expected<void, std::string> write_file(const std::filesystem::path& path,
                                            nonstd::span<const uint8_t> data,
                                            InPlace in_place = InPlace::no);
 

diff --git a/unittest/test_util_file.cpp b/unittest/test_util_file.cpp
index 798ac88f1980fefe621044c93935f3e37eec6bee..ed78dff2e8202360c676e4fa2588a38f80d17bac 100644 (file)
--- a/unittest/test_util_file.cpp
+++ b/unittest/test_util_file.cpp
@@ -40,20 +40,6 @@ namespace fs = util::filesystem;
 using TestUtil::TestContext;
 using util::DirEntry;
 
-namespace {
-
-std::string
-os_path(std::string path)
-{
-#if defined(_WIN32) && !defined(HAVE_DIRENT_H)
-  std::replace(path.begin(), path.end(), '/', '\\');
-#endif
-
-  return path;
-}
-
-} // namespace
-
 TEST_CASE("util::fallocate")
 {
   TestContext test_context;
@@ -260,8 +246,8 @@ TEST_CASE("util::traverse_directory")
   {
     CHECK_NOTHROW(util::traverse_directory("dir-with-files", visitor));
     REQUIRE(visited.size() == 3);
-    std::string f1 = os_path("[f] dir-with-files/f1");
-    std::string f2 = os_path("[f] dir-with-files/f2");
+    fs::path f1("[f] dir-with-files/f1");
+    fs::path f2("[f] dir-with-files/f2");
     CHECK(((visited[0] == f1 && visited[1] == f2)
            || (visited[0] == f2 && visited[1] == f1)));
     CHECK(visited[2] == "[d] dir-with-files");
@@ -272,8 +258,8 @@ TEST_CASE("util::traverse_directory")
     CHECK_NOTHROW(
       util::traverse_directory("dir-with-subdir-and-file", visitor));
     REQUIRE(visited.size() == 3);
-    CHECK(visited[0] == os_path("[f] dir-with-subdir-and-file/subdir/f"));
-    CHECK(visited[1] == os_path("[d] dir-with-subdir-and-file/subdir"));
+    CHECK(visited[0] == fs::path("[f] dir-with-subdir-and-file/subdir/f"));
+    CHECK(visited[1] == fs::path("[d] dir-with-subdir-and-file/subdir"));
     CHECK(visited[2] == "[d] dir-with-subdir-and-file");
   }
 }