Fix move semantics for tmp::entry (#119)

- Explicitly assign invalid value to `other` in move constructor and operator - Check that moved-out entry does not close the moved-in entry
bugdea1er · Sep 24, 2024 · 32ec319 · 32ec319
1 parent 2312f2e
commit 32ec319
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 31 deletions.
diff --git a/src/entry.cpp b/src/entry.cpp
@@ -26,6 +26,13 @@ static_assert(std::is_trivially_copyable_v<entry::native_handle_type>);
 static_assert(std::is_same_v<HANDLE, void*>);
 #endif
 
+/// Implementation-defined invalid handle to the entry
+#ifdef _WIN32
+const entry::native_handle_type invalid_handle = nullptr;
+#else
+const entry::native_handle_type invalid_handle = -1;
+#endif
+
 /// Closes the given entry, ignoring any errors
 /// @param entry     The entry to close
 void close(const entry& entry) noexcept {
@@ -69,14 +76,22 @@ entry::entry(fs::path path, native_handle_type handle)
       handle(handle) {}
 
 entry::entry(entry&& other) noexcept
-    : pathobject(other.release()),
-      handle(other.handle) {}
+    : pathobject(std::move(other.pathobject)),
+      handle(other.handle) {
+  other.pathobject.clear();
+  other.handle = invalid_handle;
+}
 
 entry& entry::operator=(entry&& other) noexcept {
   close(*this);
   remove(*this);
-  pathobject = other.release();
+
+  pathobject = std::move(other.pathobject);
+  other.pathobject.clear();
+
   handle = other.handle;
+  other.handle = invalid_handle;
+
   return *this;
 }
 

diff --git a/tests/directory.cpp b/tests/directory.cpp
@@ -134,34 +134,41 @@ TEST(directory, destructor) {
 /// Tests directory move constructor
 TEST(directory, move_constructor) {
   directory fst = directory();
-  directory snd = std::move(fst);
+  directory snd = directory(std::move(fst));
 
-  EXPECT_TRUE(fst.path().empty());
+  fst.~directory();
+
+  EXPECT_FALSE(snd.path().empty());
   EXPECT_TRUE(fs::exists(snd));
   EXPECT_TRUE(native_handle_is_valid(snd.native_handle()));
 }
 
 /// Tests directory move assignment operator
 TEST(directory, move_assignment) {
   directory fst = directory();
-  directory snd = directory();
+  {
+    directory snd = directory();
 
-  fs::path path1 = fst;
-  fs::path path2 = snd;
+    fs::path path1 = fst;
+    fs::path path2 = snd;
 
-  entry::native_handle_type fst_handle = fst.native_handle();
-  entry::native_handle_type snd_handle = snd.native_handle();
+    entry::native_handle_type fst_handle = fst.native_handle();
+    entry::native_handle_type snd_handle = snd.native_handle();
+
+    fst = std::move(snd);
 
-  fst = std::move(snd);
+    EXPECT_FALSE(fs::exists(path1));
+    EXPECT_TRUE(fs::exists(path2));
 
-  EXPECT_FALSE(fs::exists(path1));
-  EXPECT_TRUE(fs::exists(path2));
+    EXPECT_TRUE(fs::exists(fst));
+    EXPECT_TRUE(fs::equivalent(fst, path2));
 
-  EXPECT_TRUE(fs::exists(fst));
-  EXPECT_TRUE(fs::equivalent(fst, path2));
+    EXPECT_FALSE(native_handle_is_valid(fst_handle));
+    EXPECT_TRUE(native_handle_is_valid(snd_handle));
+  }
 
-  EXPECT_FALSE(native_handle_is_valid(fst_handle));
-  EXPECT_TRUE(native_handle_is_valid(snd_handle));
+  EXPECT_FALSE(fst.path().empty());
+  EXPECT_TRUE(native_handle_is_valid(fst.native_handle()));
 }
 
 /// Tests directory moving

diff --git a/tests/file.cpp b/tests/file.cpp
@@ -358,34 +358,41 @@ TEST(file, destructor) {
 /// Tests file move constructor
 TEST(file, move_constructor) {
   file fst = file();
-  file snd = std::move(fst);
+  file snd = file(std::move(fst));
 
-  EXPECT_TRUE(fst.path().empty());
+  fst.~file();
+
+  EXPECT_FALSE(snd.path().empty());
   EXPECT_TRUE(fs::exists(snd));
   EXPECT_TRUE(native_handle_is_valid(snd.native_handle()));
 }
 
 /// Tests file move assignment operator
 TEST(file, move_assignment) {
   file fst = file();
-  file snd = file();
+  {
+    file snd = file();
 
-  fs::path path1 = fst;
-  fs::path path2 = snd;
+    fs::path path1 = fst;
+    fs::path path2 = snd;
 
-  entry::native_handle_type fst_handle = fst.native_handle();
-  entry::native_handle_type snd_handle = snd.native_handle();
+    entry::native_handle_type fst_handle = fst.native_handle();
+    entry::native_handle_type snd_handle = snd.native_handle();
+
+    fst = std::move(snd);
 
-  fst = std::move(snd);
+    EXPECT_FALSE(fs::exists(path1));
+    EXPECT_TRUE(fs::exists(path2));
 
-  EXPECT_FALSE(fs::exists(path1));
-  EXPECT_TRUE(fs::exists(path2));
+    EXPECT_TRUE(fs::exists(fst));
+    EXPECT_TRUE(fs::equivalent(fst, path2));
 
-  EXPECT_TRUE(fs::exists(fst));
-  EXPECT_TRUE(fs::equivalent(fst, path2));
+    EXPECT_FALSE(native_handle_is_valid(fst_handle));
+    EXPECT_TRUE(native_handle_is_valid(snd_handle));
+  }
 
-  EXPECT_FALSE(native_handle_is_valid(fst_handle));
-  EXPECT_TRUE(native_handle_is_valid(snd_handle));
+  EXPECT_FALSE(fst.path().empty());
+  EXPECT_TRUE(native_handle_is_valid(fst.native_handle()));
 }
 
 /// Tests file moving