Fix bugs in ModifySafeMap (#14276)

src/unittest/CMakeLists.txt

@@ -9,6 +9,7 @@ set (UNITTEST_SRCS
 	${CMAKE_CURRENT_SOURCE_DIR}/test_compression.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_connection.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_craft.cpp
+	${CMAKE_CURRENT_SOURCE_DIR}/test_datastructures.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_filesys.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_inventory.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_irrptr.cpp

src/unittest/test_datastructures.cpp

@@ -0,0 +1,181 @@
+/*
+Minetest
+Copyright (C) 2024 sfan5
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU Lesser General Public License as published by
+the Free Software Foundation; either version 2.1 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU Lesser General Public License for more details.
+
+You should have received a copy of the GNU Lesser General Public License along
+with this program; if not, write to the Free Software Foundation, Inc.,
+51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+*/
+
+#include "test.h"
+
+#include "util/container.h"
+
+class TestDataStructures : public TestBase
+{
+public:
+	TestDataStructures() { TestManager::registerTestModule(this); }
+	const char *getName() { return "TestDataStructures"; }
+
+	void runTests(IGameDef *gamedef);
+
+	void testMap1();
+	void testMap2();
+	void testMap3();
+	void testMap4();
+	void testMap5();
+};
+
+static TestDataStructures g_test_instance;
+
+void TestDataStructures::runTests(IGameDef *gamedef)
+{
+	rawstream << "-------- ModifySafeMap" << std::endl;
+	TEST(testMap1);
+	TEST(testMap2);
+	TEST(testMap3);
+	TEST(testMap4);
+	TEST(testMap5);
+}
+
+namespace {
+
+struct TrackerState {
+	bool copied = false;
+	bool deleted = false;
+};
+
+class Tracker {
+	TrackerState *res = nullptr;
+
+	inline void trackDeletion() { res && (res->deleted = true); }
+
+public:
+	Tracker() {}
+	Tracker(TrackerState &res) : res(&res) {}
+
+	operator bool() const { return !!res; }
+
+	Tracker(const Tracker &other) { *this = other; }
+	Tracker &operator=(const Tracker &other) {
+		trackDeletion();
+		res = other.res;
+		res && (res->copied = true);
+		return *this;
+	}
+	Tracker(Tracker &&other) { *this = std::move(other); }
+	Tracker &operator=(Tracker &&other) {
+		trackDeletion();
+		res = other.res;
+		other.res = nullptr;
+		return *this;
+	}
+
+	~Tracker() { trackDeletion(); }
+};
+
+}
+
+void TestDataStructures::testMap1()
+{
+	ModifySafeMap<int, Tracker> map;
+	TrackerState t0, t1;
+
+	// no-copy put
+	map.put(1, Tracker(t0));
+	UASSERT(!t0.copied);
+	UASSERT(!t0.deleted);
+
+	// overwrite during iter
+	bool once = false;
+	for (auto &it : map.iter()) {
+		(void)it;
+		map.put(1, Tracker(t1));
+		UASSERT(t0.deleted);
+		UASSERT(!t1.copied);
+		UASSERT(!t1.deleted);
+		if (once |= 1)
+			break;
+	}
+	UASSERT(once);
+}
+
+void TestDataStructures::testMap2()
+{
+	ModifySafeMap<int, Tracker> map;
+	TrackerState t0, t1;
+
+	// overwrite
+	map.put(1, Tracker(t0));
+	map.put(1, Tracker(t1));
+	UASSERT(t0.deleted);
+	UASSERT(!t1.copied);
+	UASSERT(!t1.deleted);
+}
+
+void TestDataStructures::testMap3()
+{
+	ModifySafeMap<int, Tracker> map;
+	TrackerState t0, t1;
+
+	// take
+	map.put(1, Tracker(t0));
+	{
+		auto v = map.take(1);
+		UASSERT(!t0.copied);
+		UASSERT(!t0.deleted);
+	}
+	UASSERT(t0.deleted);
+
+	// remove during iter
+	map.put(1, Tracker(t1));
+	for (auto &it : map.iter()) {
+		(void)it;
+		map.remove(1);
+		UASSERT(t1.deleted);
+		break;
+	}
+}
+
+void TestDataStructures::testMap4()
+{
+	ModifySafeMap<int, u32> map;
+
+	// overwrite + take during iter
+	map.put(1, 100);
+	for (auto &it : map.iter()) {
+		(void)it;
+		map.put(1, 200);
+		u32 taken = map.take(1);
+		UASSERTEQ(u32, taken, 200);
+		break;
+	}
+
+	UASSERT(map.get(1) == u32());
+	UASSERTEQ(size_t, map.size(), 0);
+}
+
+void TestDataStructures::testMap5()
+{
+	ModifySafeMap<int, u32> map;
+
+	// overwrite 2x during iter
+	map.put(9001, 9001);
+	for (auto &it : map.iter()) {
+		(void)it;
+		map.put(1, 100);
+		map.put(1, 200);
+		UASSERTEQ(u32, map.get(1), 200);
+		break;
+	}
+}

src/util/container.h

@@ -376,10 +376,16 @@ public:
 			assert(false);
 			return;
 		}
-		if (m_iterating)
+		if (m_iterating) {
-			m_new.emplace(key, value);
+			auto it = m_values.find(key);
-		else
+			if (it != m_values.end()) {
-			m_values.emplace(key, value);
+				it->second = V();
+				m_garbage++;
+			}
+			m_new[key] = value;
+		} else {
+			m_values[key] = value;
+		}
 	}
 
 	void put(const K &key, V &&value) {
@@ -387,10 +393,16 @@ public:
 			assert(false);
 			return;
 		}
-		if (m_iterating)
+		if (m_iterating) {
-			m_new.emplace(key, std::move(value));
+			auto it = m_values.find(key);
-		else
+			if (it != m_values.end()) {
-			m_values.emplace(key, std::move(value));
+				it->second = V();
+				m_garbage++;
+			}
+			m_new[key] = std::move(value);
+		} else {
+			m_values[key] = std::move(value);
+		}
 	}
 
 	V take(const K &key) {
@@ -405,6 +417,7 @@ public:
 		auto it = m_values.find(key);
 		if (it == m_values.end())
 			return ret;
+		if (!ret)
 			ret = std::move(it->second);
 		if (m_iterating) {
 			it->second = V();
@@ -516,7 +529,8 @@ private:
 	std::map<K, V> m_values;
 	std::map<K, V> m_new;
 	unsigned int m_iterating = 0;
-	size_t m_garbage = 0; // upper bound for null-placeholders in m_values
+	// approximate amount of null-placeholders in m_values, reliable for != 0 tests
+	size_t m_garbage = 0;
 
 	static constexpr size_t GC_MIN_SIZE = 30;
 };