From da5d93b4debf0a5de86900ced24fbf5f07b59595 Mon Sep 17 00:00:00 2001
From: Johannes Altmanninger <aclopte@gmail.com>
Date: Fri, 7 Oct 2022 19:28:55 -0500
Subject: [PATCH] dir_iter_t to use unique_ptr for closing directory

dir_iter_t closes its DIR* member in two places: the move assignment and
the destructor. Simplify this by closing it in the destructor of the DIR*
member which is called in both places. Use std::unique_ptr, which is shorter
than a dedicated wrapper class. Conveniently, it calls the deleter only if
the pointer is not-null.  Unfortunately, std::unique_ptr requires explicit
conversion to DIR* when interacting with C APIs but it's probably still
better than a wrapper class.

This means that the noncopyable_t annotation is now implied due to the
unique_ptr member.
Additionally, we could probably remove the user-declared move constructor
and move assignment (the compiler-generated ones should be good enough). To
be safe, keep them around since they also erase the fd (though I hope we
don't rely on that behavior anywhere).

We should perhaps remove the user-declared destructor entirely but
dir_iter_t::entry_t also has one, I'm not sure why. Maybe there's a good
reason, like code size.

No functional change.
---
 src/wutil.cpp | 21 +++++++--------------
 src/wutil.h   |  7 +++++--
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/src/wutil.cpp b/src/wutil.cpp
index d617c5c00..f5c0a9539 100644
--- a/src/wutil.cpp
+++ b/src/wutil.cpp
@@ -173,17 +173,17 @@ void dir_iter_t::entry_t::do_stat() const {
 }
 
 dir_iter_t::dir_iter_t(const wcstring &path) {
-    dir_ = wopendir(path);
+    dir_.reset(wopendir(path));
     if (!dir_) {
         error_ = errno;
         return;
     }
-    entry_.dirfd_ = dirfd(dir_);
+    entry_.dirfd_ = dirfd(&*dir_);
 }
 
 dir_iter_t::dir_iter_t(dir_iter_t &&rhs) {
     // Steal the fields; ensure rhs no longer has FILE* and forgets its fd.
-    this->dir_ = rhs.dir_;
+    this->dir_ = std::move(rhs.dir_);
     this->error_ = rhs.error_;
     this->entry_ = std::move(rhs.entry_);
     rhs.dir_ = nullptr;
@@ -191,10 +191,7 @@ dir_iter_t::dir_iter_t(dir_iter_t &&rhs) {
 }
 
 dir_iter_t &dir_iter_t::operator=(dir_iter_t &&rhs) {
-    if (this->dir_) {
-        (void)closedir(this->dir_);
-    }
-    this->dir_ = rhs.dir_;
+    this->dir_ = std::move(rhs.dir_);
     this->error_ = rhs.error_;
     this->entry_ = std::move(rhs.entry_);
     rhs.dir_ = nullptr;
@@ -204,22 +201,18 @@ dir_iter_t &dir_iter_t::operator=(dir_iter_t &&rhs) {
 
 void dir_iter_t::rewind() {
     if (dir_) {
-        rewinddir(dir_);
+        rewinddir(&*dir_);
     }
 }
 
-dir_iter_t::~dir_iter_t() {
-    if (dir_) {
-        (void)closedir(dir_);
-    }
-}
+dir_iter_t::~dir_iter_t() = default;
 
 const dir_iter_t::entry_t *dir_iter_t::next() {
     if (!dir_) {
         return nullptr;
     }
     errno = 0;
-    struct dirent *dent = readdir(dir_);
+    struct dirent *dent = readdir(&*dir_);
     if (!dent) {
         error_ = errno;
         return nullptr;
diff --git a/src/wutil.h b/src/wutil.h
index 59d0dac2d..eacb1093e 100644
--- a/src/wutil.h
+++ b/src/wutil.h
@@ -190,7 +190,7 @@ class dir_iter_t : noncopyable_t {
     bool valid() const { return dir_ != nullptr; }
 
     /// \return the underlying file descriptor, or -1 if invalid.
-    int fd() const { return dir_ ? dirfd(dir_) : -1; }
+    int fd() const { return dir_ ? dirfd(&*dir_) : -1; }
 
     /// Rewind the directory to the beginning.
     void rewind();
@@ -247,7 +247,10 @@ class dir_iter_t : noncopyable_t {
     };
 
    private:
-    DIR *dir_{nullptr};
+    struct dir_closer_t {
+        void operator()(DIR *dir) const { (void)closedir(dir); }
+    };
+    std::unique_ptr<DIR, dir_closer_t> dir_{nullptr};
     int error_{0};
     entry_t entry_;
 };