Fix some issues with minetest.clear_craft (#8712)

* Fix some issues with minetest.clear_craft

- Fix memory leak
- Fix crafts with an output count not being cleared when clearing by
  input.
- Fix recipe list being reversed when clearing by input.

* Add CraftInput::empty()

3 files changed, 44 insertions, 74 deletions

diff --git a/src/craftdef.cpp b/src/craftdef.cpp
index 24b7437cb..0181ceb60 100644
--- a/src/craftdef.cpp
+++ b/src/craftdef.cpp
@@ -287,6 +287,15 @@ std::string craftDumpMatrix(const std::vector<ItemStack> &items,
 	CraftInput
 */
 
+bool CraftInput::empty() const
+{
+	for (const auto &item : items) {
+		if (!item.empty())
+			return false;
+	}
+	return true;
+}
+
 std::string CraftInput::dump() const
 {
 	std::ostringstream os(std::ios::binary);
@@ -906,18 +915,7 @@ public:
 			std::vector<ItemStack> &output_replacement, bool decrementInput,
 			IGameDef *gamedef) const
 	{
-		output.item = "";
-		output.time = 0;
-
-		// If all input items are empty, abort.
-		bool all_empty = true;
-		for (const auto &item : input.items) {
-			if (!item.empty()) {
-				all_empty = false;
-				break;
-			}
-		}
-		if (all_empty)
+		if (input.empty())
 			return false;
 
 		std::vector<std::string> input_names;
@@ -1002,84 +1000,48 @@ public:
 		return recipes;
 	}
 
-	virtual bool clearCraftRecipesByOutput(const CraftOutput &output, IGameDef *gamedef)
+	virtual bool clearCraftsByOutput(const CraftOutput &output, IGameDef *gamedef)
 	{
-		auto vec_iter = m_output_craft_definitions.find(output.item);
+		auto to_clear = m_output_craft_definitions.find(output.item);
 
-		if (vec_iter == m_output_craft_definitions.end())
+		if (to_clear == m_output_craft_definitions.end())
 			return false;
 
-		std::vector<CraftDefinition*> &vec = vec_iter->second;
-		for (auto def : vec) {
+		for (auto def : to_clear->second) {
 			// Recipes are not yet hashed at this point
-			std::vector<CraftDefinition*> &unhashed_inputs_vec = m_craft_defs[(int) CRAFT_HASH_TYPE_UNHASHED][0];
-			std::vector<CraftDefinition*> new_vec_by_input;
-			/* We will preallocate necessary memory addresses, so we don't need to reallocate them later.
-				This would save us some performance. */
-			new_vec_by_input.reserve(unhashed_inputs_vec.size());
-			for (auto &i2 : unhashed_inputs_vec) {
-				if (def != i2) {
-					new_vec_by_input.push_back(i2);
-				}
-			}
-			m_craft_defs[(int) CRAFT_HASH_TYPE_UNHASHED][0].swap(new_vec_by_input);
+			std::vector<CraftDefinition *> &defs = m_craft_defs[(int)CRAFT_HASH_TYPE_UNHASHED][0];
+			defs.erase(std::remove(defs.begin(), defs.end(), def), defs.end());
+			delete def;
 		}
-		m_output_craft_definitions.erase(output.item);
+		m_output_craft_definitions.erase(to_clear);
 		return true;
 	}
 
-	virtual bool clearCraftRecipesByInput(CraftMethod craft_method, unsigned int craft_grid_width,
-		const std::vector<std::string> &recipe, IGameDef *gamedef)
+	virtual bool clearCraftsByInput(const CraftInput &input, IGameDef *gamedef)
 	{
-		bool all_empty = true;
-		for (const auto &i : recipe) {
-			if (!i.empty()) {
-				all_empty = false;
-				break;
-			}
-		}
-		if (all_empty)
+		if (input.empty())
 			return false;
 
-		CraftInput input(craft_method, craft_grid_width, craftGetItems(recipe, gamedef));
 		// Recipes are not yet hashed at this point
-		std::vector<CraftDefinition*> &unhashed_inputs_vec = m_craft_defs[(int) CRAFT_HASH_TYPE_UNHASHED][0];
-		std::vector<CraftDefinition*> new_vec_by_input;
+		std::vector<CraftDefinition *> &defs = m_craft_defs[(int)CRAFT_HASH_TYPE_UNHASHED][0];
+		std::vector<CraftDefinition *> new_defs;
 		bool got_hit = false;
-		for (std::vector<CraftDefinition*>::size_type
-				i = unhashed_inputs_vec.size(); i > 0; i--) {
-			CraftDefinition *def = unhashed_inputs_vec[i - 1];
-			/* If the input doesn't match the recipe definition, this recipe definition later
-				will be added back in source map. */
+		for (auto def : defs) {
 			if (!def->check(input, gamedef)) {
-				new_vec_by_input.push_back(def);
+				new_defs.push_back(def);
 				continue;
 			}
-			CraftOutput output = def->getOutput(input, gamedef);
 			got_hit = true;
-			auto vec_iter = m_output_craft_definitions.find(output.item);
-			if (vec_iter == m_output_craft_definitions.end())
+			std::string output = def->getOutput(input, gamedef).item;
+			delete def;
+			auto it = m_output_craft_definitions.find(craftGetItemName(output, gamedef));
+			if (it == m_output_craft_definitions.end())
 				continue;
-			std::vector<CraftDefinition*> &vec = vec_iter->second;
-			std::vector<CraftDefinition*> new_vec_by_output;
-			/* We will preallocate necessary memory addresses, so we don't need
-				to reallocate them later. This would save us some performance. */
-			new_vec_by_output.reserve(vec.size());
-			for (auto &vec_i : vec) {
-				/* If pointers from map by input and output are not same,
-					we will add 'CraftDefinition*' to a new vector. */
-				if (def != vec_i) {
-					/* Adding dereferenced iterator value (which are
-						'CraftDefinition' reference) to a new vector. */
-					new_vec_by_output.push_back(vec_i);
-				}
-			}
-			// Swaps assigned to current key value with new vector for output map.
-			m_output_craft_definitions[output.item].swap(new_vec_by_output);
+			std::vector<CraftDefinition *> &outdefs = it->second;
+			outdefs.erase(std::remove(outdefs.begin(), outdefs.end(), def), outdefs.end());
 		}
 		if (got_hit)
-			// Swaps value with new vector for input map.
-			m_craft_defs[(int) CRAFT_HASH_TYPE_UNHASHED][0].swap(new_vec_by_input);
+			defs.swap(new_defs);
 
 		return got_hit;
 	}
diff --git a/src/craftdef.h b/src/craftdef.h
index 37ae6df43..5971a89bf 100644
--- a/src/craftdef.h
+++ b/src/craftdef.h
@@ -80,6 +80,9 @@ struct CraftInput
 		method(method_), width(width_), items(items_)
 	{}
 
+	// Returns true if all items are empty.
+	bool empty() const;
+
 	std::string dump() const;
 };
 
@@ -431,9 +434,8 @@ public:
 	virtual std::vector<CraftDefinition*> getCraftRecipes(CraftOutput &output,
 			IGameDef *gamedef, unsigned limit=0) const=0;
 
-	virtual bool clearCraftRecipesByOutput(const CraftOutput &output, IGameDef *gamedef) = 0;
-	virtual bool clearCraftRecipesByInput(CraftMethod craft_method,
-			unsigned int craft_grid_width, const std::vector<std::string> &recipe, IGameDef *gamedef) = 0;
+	virtual bool clearCraftsByOutput(const CraftOutput &output, IGameDef *gamedef) = 0;
+	virtual bool clearCraftsByInput(const CraftInput &input, IGameDef *gamedef) = 0;
 
 	// Print crafting recipes for debugging
 	virtual std::string dump() const=0;
diff --git a/src/script/lua_api/l_craft.cpp b/src/script/lua_api/l_craft.cpp
index 0899b945e..18622ee00 100644
--- a/src/script/lua_api/l_craft.cpp
+++ b/src/script/lua_api/l_craft.cpp
@@ -294,7 +294,7 @@ int ModApiCraft::l_clear_craft(lua_State *L)
 	std::string type = getstringfield_default(L, table, "type", "shaped");
 	CraftOutput c_output(output, 0);
 	if (!output.empty()) {
-		if (craftdef->clearCraftRecipesByOutput(c_output, getServer(L))) {
+		if (craftdef->clearCraftsByOutput(c_output, getServer(L))) {
 			lua_pushboolean(L, true);
 			return 1;
 		}
@@ -351,7 +351,13 @@ int ModApiCraft::l_clear_craft(lua_State *L)
 		throw LuaError("Unknown crafting definition type: \"" + type + "\"");
 	}
 
-	if (!craftdef->clearCraftRecipesByInput(method, width, recipe, getServer(L))) {
+	std::vector<ItemStack> items;
+	items.reserve(recipe.size());
+	for (const auto &item : recipe)
+		items.emplace_back(item, 1, 0, getServer(L)->idef());
+	CraftInput input(method, width, items);
+
+	if (!craftdef->clearCraftsByInput(input, getServer(L))) {
 		warningstream << "No craft recipe matches input" << std::endl;
 		lua_pushboolean(L, false);
 		return 1;