Fix issues reported by Sonar, review fixes

AI/Nullkiller/Engine/PriorityEvaluator.cpp

@@ -1220,7 +1220,7 @@ public:
 		}
 		else if(bi.id >= BuildingID::MAGES_GUILD_1 && bi.id <= BuildingID::MAGES_GUILD_5)
 		{
-			evaluationContext.skillReward += 2 * buildThis.town->spellsAtLevel(bi.id.getMagesGuildLevel(), false);
+			evaluationContext.skillReward += 2 * bi.id.getMagesGuildLevel();
 			if (!alreadyOwn && evaluationContext.evaluator.ai->cb->canBuildStructure(buildThis.town, highestMageGuildPossible) != EBuildingState::FORBIDDEN)
 			{
 				for (auto hero : evaluationContext.evaluator.ai->cb->getHeroesInfo())

AI/Nullkiller/Pathfinding/Actions/QuestAction.cpp

@@ -52,7 +52,7 @@ namespace AIPathfinding
 
 	void QuestAction::execute(AIGateway * ai, const CGHeroInstance * hero) const
 	{
-		ai->moveHeroToTile(questInfo.getObject(cb)->visitablePos(), hero);
+		ai->moveHeroToTile(questInfo.getObject(ai->myCb.get())->visitablePos(), hero);
 	}
 
 	std::string QuestAction::toString() const

lib/CGameInfoCallback.cpp

@@ -184,11 +184,9 @@ const IMarket * CGameInfoCallback::getMarket(ObjectInstanceID objid) const
 
 void CGameInfoCallback::fillUpgradeInfo(const CArmedInstance *obj, SlotID stackPos, UpgradeInfo & out) const
 {
-	//std::shared_lock<std::shared_mutex> lock(*gameState().mx);
 	ERROR_RET_IF(!canGetFullInfo(obj), "Cannot get info about not owned object!");
 	ERROR_RET_IF(!obj->hasStackAtSlot(stackPos), "There is no such stack!");
 	gameState().fillUpgradeInfo(obj, stackPos, out);
-	//return gameState().fillUpgradeInfo(obj->getStack(stackPos));
 }
 
 const StartInfo * CGameInfoCallback::getStartInfo() const
@@ -203,7 +201,6 @@ const StartInfo * CGameInfoCallback::getInitialStartInfo() const
 
 int32_t CGameInfoCallback::getSpellCost(const spells::Spell * sp, const CGHeroInstance * caster) const
 {
-	//std::shared_lock<std::shared_mutex> lock(*gameState().mx);
 	ERROR_RET_VAL_IF(!canGetFullInfo(caster), "Cannot get info about caster!", -1);
 	//if there is a battle
 	auto casterBattle = gameState().getBattle(caster->getOwner());
@@ -217,8 +214,6 @@ int32_t CGameInfoCallback::getSpellCost(const spells::Spell * sp, const CGHeroIn
 
 int64_t CGameInfoCallback::estimateSpellDamage(const CSpell * sp, const CGHeroInstance * hero) const
 {
-	//std::shared_lock<std::shared_mutex> lock(*gameState().mx);
-
 	ERROR_RET_VAL_IF(hero && !canGetFullInfo(hero), "Cannot get info about caster!", -1);
 
 	if(hero) //we see hero's spellbook
@@ -229,7 +224,6 @@ int64_t CGameInfoCallback::estimateSpellDamage(const CSpell * sp, const CGHeroIn
 
 void CGameInfoCallback::getThievesGuildInfo(SThievesGuildInfo & thi, const CGObjectInstance * obj)
 {
-	//std::shared_lock<std::shared_mutex> lock(*gameState().mx);
 	ERROR_RET_IF(!obj, "No guild object!");
 	ERROR_RET_IF(obj->ID == Obj::TOWN && !canGetFullInfo(obj), "Cannot get info about town guild object!");
 	//TODO: advmap object -> check if they're visited by our hero
@@ -419,13 +413,11 @@ bool CGameInfoCallback::getHeroInfo(const CGObjectInstance * hero, InfoAboutHero
 
 int CGameInfoCallback::getDate(Date mode) const
 {
-	//std::shared_lock<std::shared_mutex> lock(*gameState().mx);
 	return gameState().getDate(mode);
 }
 
 bool CGameInfoCallback::isVisible(int3 pos, const std::optional<PlayerColor> & Player) const
 {
-	//std::shared_lock<std::shared_mutex> lock(*gameState().mx);
 	return gameState().isVisible(pos, Player);
 }
 
@@ -714,14 +706,12 @@ bool CGameInfoCallback::isPlayerMakingTurn(PlayerColor player) const
 
 int CPlayerSpecificInfoCallback::howManyTowns() const
 {
-	//std::shared_lock<std::shared_mutex> lock(*gameState().mx);
 	ERROR_RET_VAL_IF(!getPlayerID(), "Applicable only for player callbacks", -1);
 	return CGameInfoCallback::howManyTowns(*getPlayerID());
 }
 
 std::vector < const CGTownInstance *> CPlayerSpecificInfoCallback::getTownsInfo(bool onlyOur) const
 {
-	//std::shared_lock<std::shared_mutex> lock(*gameState().mx);
 	auto ret = std::vector < const CGTownInstance *>();
 	for(const auto & i : gameState().players)
 	{
@@ -790,7 +780,6 @@ std::vector <QuestInfo> CPlayerSpecificInfoCallback::getMyQuests() const
 
 int CPlayerSpecificInfoCallback::howManyHeroes(bool includeGarrisoned) const
 {
-	//std::shared_lock<std::shared_mutex> lock(*gameState().mx);
 	ERROR_RET_VAL_IF(!getPlayerID(), "Applicable only for player callbacks", -1);
 	return getHeroCount(*getPlayerID(), includeGarrisoned);
 }
@@ -822,14 +811,12 @@ const CGTownInstance* CPlayerSpecificInfoCallback::getTownBySerial(int serialId)
 
 int CPlayerSpecificInfoCallback::getResourceAmount(GameResID type) const
 {
-	//std::shared_lock<std::shared_mutex> lock(*gameState().mx);
 	ERROR_RET_VAL_IF(!getPlayerID(), "Applicable only for player callbacks", -1);
 	return getResource(*getPlayerID(), type);
 }
 
 TResources CPlayerSpecificInfoCallback::getResourceAmount() const
 {
-	//std::shared_lock<std::shared_mutex> lock(*gameState().mx);
 	ERROR_RET_VAL_IF(!getPlayerID(), "Applicable only for player callbacks", TResources());
 	return gameState().players.at(*getPlayerID()).resources;
 }

lib/battle/BattleInfo.cpp

@@ -961,7 +961,7 @@ CGHeroInstance * BattleInfo::battleGetFightingHero(BattleSide side) const
 
 void BattleInfo::postDeserialize()
 {
-	for (auto & unit : stacks)
+	for (const auto & unit : stacks)
 		unit->postDeserialize(getSideArmy(unit->unitSide()));
 }
 

lib/constants/EntityIdentifiers.h

@@ -358,9 +358,7 @@ public:
 	static std::string encode(int32_t index);
 	static si32 decode(const std::string & identifier);
 
-public:
-
-	int getMagesGuildLevel()
+	int getMagesGuildLevel() const
 	{
 		switch (toEnum())
 		{

lib/gameState/CGameState.cpp

@@ -637,9 +637,9 @@ void CGameState::initHeroes()
 			auto newHeroPtr = std::make_shared<CGHeroInstance>(cb);
 			newHeroPtr->subID = htype.getNum();
 			map->addToHeroPool(newHeroPtr);
-			map->generateUniqueInstanceName(newHeroPtr.get());
 			heroInPool = newHeroPtr.get();
 		}
+		map->generateUniqueInstanceName(heroInPool);
 		heroInPool->initHero(getRandomGenerator());
 		heroesPool->addHeroToPool(htype);
 	}

lib/gameState/CGameState.h

@@ -116,7 +116,12 @@ public:
 	ArtifactID pickRandomArtifact(vstd::RNG & rand, int flags, std::function<bool(ArtifactID)> accepts);
 	ArtifactID pickRandomArtifact(vstd::RNG & rand, std::set<ArtifactID> filtered);
 
+	/// Creates instance of spell scroll artifact with provided spell
 	CArtifactInstance * createScroll(const SpellID & spellId);
+
+	/// Creates instance of requested artifact
+	/// For combined artifact this method will also create alll required components
+	/// For scrolls this method will also initialize its spell
 	CArtifactInstance * createArtifact(const ArtifactID & artId, const SpellID & spellId = SpellID::NONE);
 
 	/// Returns battle in which selected player is engaged, or nullptr if none.

lib/gameState/CGameStateCampaign.cpp

@@ -379,7 +379,6 @@ void CGameStateCampaign::replaceHeroesPlaceholders()
 		if(heroPlaceholder->tempOwner.isValidPlayer())
 			heroToPlace->tempOwner = heroPlaceholder->tempOwner;
 
-		// FIXME: consider whether to move these actions to CMap::replaceObject method
 		heroToPlace->setAnchorPos(heroPlaceholder->anchorPos());
 		heroToPlace->setHeroType(heroToPlace->getHeroTypeID());
 		heroToPlace->appearance = heroToPlace->getObjectHandler()->getTemplates().front();
@@ -436,9 +435,6 @@ void CGameStateCampaign::transferMissingArtifacts(const CampaignTravel & travelO
 			else
 				logGlobal->error("Cannot transfer artifact - no receiver hero!");
 		}
-
-		// FIXME: erase entry from array? clear entire campaignHeroReplacements?
-		//campaignHeroReplacement.hero.reset();
 	}
 }
 

lib/gameState/GameStatistics.cpp

@@ -334,7 +334,7 @@ std::map<EGameResID, int> Statistic::getNumMines(const CGameState * gs, const Pl
 	for(const auto * object : ps->getOwnedObjects())
 	{
 		//Mines
-		if ( object->ID == Obj::MINE )
+		if(object->ID == Obj::MINE || object->ID == Obj::ABANDONED_MINE)
 		{
 			const auto * mine = dynamic_cast<const CGMine *>(object);
 			assert(mine);

lib/mapObjects/CGMarket.cpp

@@ -78,8 +78,8 @@ std::set<EMarketMode> CGMarket::availableModes() const
 }
 
 CGMarket::CGMarket(IGameCallback *cb)
-	: IMarket(cb)
-	, CGObjectInstance(cb)
+	: CGObjectInstance(cb)
+	, IMarket(cb)
 {}
 
 std::vector<TradeItemBuy> CGBlackMarket::availableItemsIds(EMarketMode mode) const

lib/mapObjects/CQuest.cpp

@@ -496,7 +496,7 @@ void CGSeerHut::initObj(vstd::RNG & rand)
 
 void CGSeerHut::getRolloverText(MetaString &text, bool onHover) const
 {
-	getQuest().getRolloverText(cb, text, onHover);//TODO: simplify?
+	getQuest().getRolloverText(cb, text, onHover);
 	if(!onHover)
 		text.replaceRawString(seerName);
 }

lib/mapObjects/MiscObjects.cpp

@@ -665,21 +665,16 @@ void CGArtifact::initObj(vstd::RNG & rand)
 	blockVisit = true;
 	if(ID == Obj::ARTIFACT)
 	{
+		assert(getArtifactType().hasValue());
+
 		if (!storedArtifact.hasValue())
-			setArtifactInstance(cb->gameState().createArtifact(ArtifactID()));
-
-		auto * artifact = cb->gameState().getArtInstance(storedArtifact);
-
-		if(!artifact->getType())
-			artifact->setType(getArtifactType().toArtifact());
+			setArtifactInstance(cb->gameState().createArtifact(getArtifactType()));
 	}
 	if(ID == Obj::SPELL_SCROLL)
 		subID = 1;
 
 	assert(getArtifactInstance()->getType());
 	assert(!getArtifactInstance()->getParentNodes().empty());
-
-	//assert(storedArtifact->artType->id == subID); //this does not stop desync
 }
 
 std::string CGArtifact::getObjectName() const

lib/mapping/CMap.cpp

@@ -811,7 +811,7 @@ CArtifactInstance * CMap::createScroll(const SpellID & spellId)
 	return createArtifact(ArtifactID::SPELL_SCROLL, spellId);
 }
 
-CArtifactInstance * CMap::createSingleArtifact(const ArtifactID & artId, const SpellID & spellId)
+CArtifactInstance * CMap::createArtifactComponent(const ArtifactID & artId)
 {
 	auto newArtifact = artId.hasValue() ?
 		std::make_shared<CArtifactInstance>(cb, artId.toArtifact()):
@@ -825,15 +825,11 @@ CArtifactInstance * CMap::createSingleArtifact(const ArtifactID & artId, const S
 CArtifactInstance * CMap::createArtifact(const ArtifactID & artID, const SpellID & spellId)
 {
 	if(!artID.hasValue())
-	{
-		// random, empty
-		// TODO: make this illegal & remove? Such artifact can't be randomized as combined artifact later
-		return createSingleArtifact(artID, spellId);
-	}
+		throw std::runtime_error("Can't create empty artifact!");
 
 	auto art = artID.toArtifact();
 
-	auto artInst = createSingleArtifact(artID, spellId);
+	auto artInst = createArtifactComponent(artID);
 	if(art->isCombined() && !art->isFused())
 	{
 		for(const auto & part : art->getConstituents())
@@ -874,18 +870,6 @@ const std::vector<ObjectInstanceID> & CMap::getHeroesOnMap()
 	return heroesOnMap;
 }
 
-void CMap::postInitialize()
-{
-	//TODO: check whether this is actually needed
-	boost::range::sort(heroesOnMap, [this](const ObjectInstanceID & a, const ObjectInstanceID & b)
-	{
-		const auto aHero = std::dynamic_pointer_cast<const CGHeroInstance>(objects.at(a.getNum()));
-		const auto bHero = std::dynamic_pointer_cast<const CGHeroInstance>(objects.at(b.getNum()));
-
-		return aHero->getHeroTypeID() < bHero->getHeroTypeID();
-	});
-}
-
 void CMap::addToHeroPool(std::shared_ptr<CGHeroInstance> hero)
 {
 	assert(hero->getHeroTypeID().isValid());

lib/mapping/CMap.h

@@ -72,7 +72,7 @@ class DLL_LINKAGE CMap : public CMapHeader, public GameCallbackHolder
 	/// Precomputed indices of all towns on map
 	std::vector<ObjectInstanceID> towns;
 
-	/// Precomputed indices of all towns on map. Does not includes heroes in prisons
+	/// Precomputed indices of all heroes on map. Does not includes heroes in prisons
 	std::vector<ObjectInstanceID> heroesOnMap;
 
 public:
@@ -97,26 +97,50 @@ public:
 	void calculateGuardingGreaturePositions();
 
 	void saveCompatibilityAddMissingArtifact(std::shared_ptr<CArtifactInstance> artifact);
-	CArtifactInstance * createScroll(const SpellID & spellId);
-	CArtifactInstance * createArtifact(const ArtifactID & artId, const SpellID & spellId = SpellID::NONE);
-	CArtifactInstance * createSingleArtifact(const ArtifactID & artId, const SpellID & spellId = SpellID::NONE);
 
+	/// Creates instance of spell scroll artifact with provided spell
+	CArtifactInstance * createScroll(const SpellID & spellId);
+
+	/// Creates instance of requested artifact
+	/// For combined artifact this method will also create alll required components
+	/// For scrolls this method will also initialize its spell
+	CArtifactInstance * createArtifact(const ArtifactID & artId, const SpellID & spellId = SpellID::NONE);
+
+	/// Creates single instance of requested artifact
+	/// Does NOT creates components for combined artifacts
+	/// Does NOT initializes spell when spell scroll artifact is created
+	CArtifactInstance * createArtifactComponent(const ArtifactID & artId);
+
+	/// Returns pointer to requested Artifact Instance. Throws on invalid ID
 	CArtifactInstance * getArtifactInstance(const ArtifactInstanceID & artifactID);
+	/// Returns pointer to requested Artifact Instance. Throws on invalid ID
 	const CArtifactInstance * getArtifactInstance(const ArtifactInstanceID & artifactID) const;
 
+	/// Completely removes artifact instance from the game
 	void eraseArtifactInstance(const ArtifactInstanceID art);
+
 	void moveArtifactInstance(CArtifactSet & srcSet, const ArtifactPosition & srcSlot, CArtifactSet & dstSet, const ArtifactPosition & dstSlot);
 	void putArtifactInstance(CArtifactSet & set, const ArtifactInstanceID art, const ArtifactPosition & slot);
 	void removeArtifactInstance(CArtifactSet & set, const ArtifactPosition & slot);
 
+	/// Generates unique string identifier for provided object instance
 	void generateUniqueInstanceName(CGObjectInstance * target);
+
+	/// Generates new, unique numeric identifier that can be used for creation of a new object
 	ObjectInstanceID allocateUniqueInstanceID();
 
-	///Use only this method when creating new map object instances
+	/// Adds provided object to the map
+	/// Throws on error, for example if object is already on map
 	void addNewObject(std::shared_ptr<CGObjectInstance> obj);
+
+	/// Moves anchor position of requested object to specified coordinates and updates map state
+	/// Throws in invalid object instance ID
 	void moveObject(ObjectInstanceID target, const int3 & dst);
 
+	/// Hides object from map without actually removing it from object list
 	void hideObject(CGObjectInstance * obj);
+
+	/// Shows previously hiiden object on map
 	void showObject(CGObjectInstance * obj);
 
 	/// Remove objects and shifts object indicies.
@@ -137,9 +161,17 @@ public:
 	void townAddedToMap(const CGTownInstance * town);
 	void townRemovedFromMap(const CGTownInstance * town);
 
+	/// Adds provided hero to map pool. Hero with same identity must not exist
 	void addToHeroPool(std::shared_ptr<CGHeroInstance> hero);
+
+	/// Attempts to take hero of specified identity from pool. Returns nullptr on failure
+	/// Hero is removed from pool on success
 	std::shared_ptr<CGHeroInstance> tryTakeFromHeroPool(HeroTypeID hero);
+
+	/// Attempts to access hero of specified identity in pool. Returns nullptr on failure
 	CGHeroInstance * tryGetFromHeroPool(HeroTypeID hero);
+
+	/// Returns list of identities of heroes currently present in pool
 	std::vector<HeroTypeID> getHeroesInPool() const;
 
 	CGObjectInstance * getObject(ObjectInstanceID obj);
@@ -147,6 +179,7 @@ public:
 
 	void attachToBonusSystem(CGameState & gs);
 
+	/// Returns all valid objects of specified class present on map
 	template<typename ObjectType = CGObjectInstance>
 	std::vector<const ObjectType *> getObjects() const
 	{
@@ -160,6 +193,7 @@ public:
 		return result;
 	}
 
+	/// Returns all valid objects of specified class present on map
 	template<typename ObjectType = CGObjectInstance>
 	std::vector<ObjectType *> getObjects()
 	{
@@ -173,6 +207,7 @@ public:
 		return result;
 	}
 
+	/// Returns all valid artifacts present on map
 	std::vector<CArtifactInstance *> getArtifacts()
 	{
 		std::vector<CArtifactInstance *> result;
@@ -242,7 +277,6 @@ public:
 	void overrideGameSetting(EGameSettings option, const JsonNode & input);
 	const IGameSettings & getSettings() const;
 
-	void postInitialize();
 	void saveCompatibilityStoreAllocatedArtifactID();
 
 private:

lib/mapping/MapFormatH3M.cpp

@@ -1407,7 +1407,9 @@ std::shared_ptr<CGObjectInstance> CMapLoaderH3M::readArtifact(const int3 & mapPo
 			logGlobal->warn("Map '%s': Artifact %s: not implemented pickup mode %d (flags: %d)", mapName, mapPosition.toString(), pickupMode, static_cast<int>(pickupFlags));
 	}
 
-	object->setArtifactInstance(map->createArtifact(artID, SpellID::NONE));
+	if (artID.hasValue())
+		object->setArtifactInstance(map->createArtifact(artID, SpellID::NONE));
+	// else - random, will be initialized later
 	return object;
 }
 
@@ -1956,8 +1958,6 @@ void CMapLoaderH3M::readObjects()
 		map->generateUniqueInstanceName(newObject.get());
 		map->addNewObject(newObject);
 	}
-
-	map->postInitialize();
 }
 
 void CMapLoaderH3M::readCreatureSet(CArmedInstance * out, const ObjectInstanceID & idToBeGiven)

lib/mapping/MapFormatJson.cpp

@@ -1121,8 +1121,6 @@ void CMapLoaderJson::readObjects()
 	for(auto & ptr : loaders)
 		ptr->configure();
 
-	map->postInitialize();
-
 	std::set<HeroTypeID> debugHeroesOnMap;
 	for (auto const & hero : map->getObjects<CGHeroInstance>())
 	{

lib/mapping/ObstacleProxy.cpp

@@ -244,7 +244,7 @@ int ObstacleProxy::getWeightedObjects(const int3 & tile, vstd::RNG & rand, IGame
 		for(const auto & temp : shuffledObstacles)
 		{
 			auto handler = LIBRARY->objtypeh->getHandlerFor(temp->id, temp->subid);
-			auto obj = handler->create(nullptr, temp);
+			auto obj = handler->create(cb, temp);
 			allObjects.emplace_back(obj);
 			rmg::Object * rmgObject = &allObjects.back();
 			for(const auto & offset : obj->getBlockedOffsets())

lib/networkPacks/NetPacksLib.cpp

@@ -1779,7 +1779,7 @@ void AssembledArtifact::applyGs(CGameState *gs)
 			return art->getId() == builtArt->getId();
 		}));
 
-	auto * combinedArt = gs->getMap().createSingleArtifact(artId);
+	auto * combinedArt = gs->getMap().createArtifactComponent(artId);
 
 	// Find slots for all involved artifacts
 	std::set<ArtifactPosition, std::greater<>> slotsInvolved = { al.slot };

lib/serializer/CLoadFile.cpp

@@ -21,33 +21,31 @@ struct static_caster
 
 CLoadFile::CLoadFile(const boost::filesystem::path & fname, IGameCallback * cb)
 	: serializer(this)
-	, fName(fname.string())
 	, sfile(fname.c_str(), std::ios::in | std::ios::binary)
 {
 	serializer.cb = cb;
 	serializer.loadingGamestate = true;
 	assert(!serializer.reverseEndianness);
 
-	fName = fname.string();
 	sfile.exceptions(std::ifstream::failbit | std::ifstream::badbit); //we throw a lot anyway
 
 	if(!sfile)
-		throw std::runtime_error("Error: cannot open file '" + fName + "' for reading!");
+		throw std::runtime_error("Error: cannot open file '" + fname.string() + "' for reading!");
 
-	//we can read
-	char magic[4];
-	sfile.read(magic, 4);
-	if(std::memcmp(magic, "VCMI", 4) != 0)
-		throw std::runtime_error("Error: '" + fName + "' is not a VCMI file!");
+	static const std::string MAGIC = "VCMI";
+	std::string readMagic = MAGIC;
+	sfile.read(readMagic.data(), 4);
+	if(readMagic != MAGIC)
+		throw std::runtime_error("Error: '" + fname.string() + "' is not a VCMI file!");
 
 	sfile.read(reinterpret_cast<char*>(&serializer.version), sizeof(serializer.version));
 
 	if(serializer.version < ESerializationVersion::MINIMAL)
-		throw std::runtime_error("Error: too old file format detected in '" + fName + "'!");
+		throw std::runtime_error("Error: too old file format detected in '" + fname.string() + "'!");
 
 	if(serializer.version > ESerializationVersion::CURRENT)
 	{
-		logGlobal->warn("Warning format version mismatch: found %d when current is %d! (file %s)\n", vstd::to_underlying(serializer.version), vstd::to_underlying(ESerializationVersion::CURRENT), fName);
+		logGlobal->warn("Warning format version mismatch: found %d when current is %d! (file %s)\n", vstd::to_underlying(serializer.version), vstd::to_underlying(ESerializationVersion::CURRENT), fname.string());
 
 		auto * versionptr = reinterpret_cast<char *>(&serializer.version);
 		std::reverse(versionptr, versionptr + 4);
@@ -59,7 +57,7 @@ CLoadFile::CLoadFile(const boost::filesystem::path & fname, IGameCallback * cb)
 			serializer.reverseEndianness = true;
 		}
 		else
-			throw std::runtime_error("Error: too new file format detected in '" + fName + "'!");
+			throw std::runtime_error("Error: too new file format detected in '" + fname.string() + "'!");
 	}
 
 	std::string loaded = SAVEGAME_MAGIC;

lib/serializer/CLoadFile.h

@@ -16,8 +16,6 @@ VCMI_LIB_NAMESPACE_BEGIN
 class DLL_LINKAGE CLoadFile : public IBinaryReader
 {
 	BinaryDeserializer serializer;
-
-	std::string fName;
 	std::fstream sfile;
 
 	int read(std::byte * data, unsigned size) override; //throws!
@@ -28,7 +26,6 @@ public:
 	template<class T>
 	void load(T & data)
 	{
-		//static_assert(is_serializeable<BinaryDeserializer, T>::value, "This class can't be deserialized (possible pointer?)");
 		serializer & data;
 	}
 

lib/serializer/CSaveFile.cpp

@@ -15,12 +15,11 @@ VCMI_LIB_NAMESPACE_BEGIN
 CSaveFile::CSaveFile(const boost::filesystem::path &fname)
 	: serializer(this)
 	, sfile(fname.c_str(), std::ios::out | std::ios::binary)
-	, fName(fname)
 {
 	sfile.exceptions(std::ifstream::failbit | std::ifstream::badbit); //we throw a lot anyway
 
 	if(!sfile)
-		throw std::runtime_error("Error: cannot open file '" + fName.string() + "' for writing!");
+		throw std::runtime_error("Error: cannot open file '" + fname.string() + "' for writing!");
 
 	sfile.write("VCMI", 4); //write magic identifier
 	serializer & ESerializationVersion::CURRENT; //write format version

lib/serializer/CSaveFile.h

@@ -17,8 +17,6 @@ VCMI_LIB_NAMESPACE_BEGIN
 class DLL_LINKAGE CSaveFile final : public IBinaryWriter
 {
 	BinarySerializer serializer;
-
-	boost::filesystem::path fName;
 	std::fstream sfile;
 
 	int write(const std::byte * data, unsigned size) final;

server/CGameHandler.cpp

@@ -433,7 +433,7 @@ void CGameHandler::handleClientDisconnection(std::shared_ptr<CConnection> c)
 	for(auto & playerConnections : connections)
 	{
 		PlayerColor playerId = playerConnections.first;
-		auto * playerSettings = gameState().getStartInfo()->getPlayersSettings(playerId.getNum());
+		const auto * playerSettings = gameState().getStartInfo()->getPlayersSettings(playerId.getNum());
 		if(!playerSettings)
 			continue;
 		
@@ -554,11 +554,11 @@ void CGameHandler::init(StartInfo *si, Load::ProgressAccumulator & progressTrack
 	for (const auto & elem : gameState().players)
 		turnOrder->addPlayer(elem.first);
 
-//	for (auto & elem : gameState().getMap().allHeroes)
-//	{
-//		if(elem)
-//			heroPool->getHeroSkillsRandomGenerator(elem->getHeroTypeID()); // init RMG seed
-//	}
+	for (const auto & elem : gameState().getMap().getObjects<CGHeroInstance>())
+		heroPool->getHeroSkillsRandomGenerator(elem->getHeroTypeID()); // init RMG seed
+
+	for (const auto & elem : gameState().getMap().getHeroesInPool())
+		heroPool->getHeroSkillsRandomGenerator(elem); // init RMG seed
 
 	reinitScripting();
 }
@@ -674,9 +674,9 @@ void CGameHandler::onNewTurn()
 	for (const auto & townID : gameState().getMap().getAllTowns())
 	{
 		auto t = gameState().getTown(townID);
-		if (t->hasBonusOfType (BonusType::DARKNESS))
+		if(t->hasBonusOfType(BonusType::DARKNESS))
 		{
-			for (auto & player : gameState().players)
+			for(const auto & player : gameState().players)
 			{
 				if (getPlayerStatus(player.first) == EPlayerStatus::INGAME &&
 					getPlayerRelations(player.first, t->tempOwner) == PlayerRelations::ENEMIES)
@@ -733,7 +733,7 @@ void CGameHandler::start(bool resume)
 	{
 		onNewTurn();
 		events::TurnStarted::defaultExecute(serverEventBus.get());
-		for(auto & player : gameState().players)
+		for(const auto & player : gameState().players)
 			turnTimerHandler->onGameplayStart(player.first);
 	}
 	else

server/processors/PlayerMessageProcessor.cpp

@@ -127,12 +127,12 @@ void PlayerMessageProcessor::commandSave(PlayerColor player, const std::vector<s
 void PlayerMessageProcessor::commandCheaters(PlayerColor player, const std::vector<std::string> & words)
 {
 	int playersCheated = 0;
-	for(const auto & player : gameHandler->gameState().players)
+	for(const auto & playerState : gameHandler->gameState().players)
 	{
-		if(player.second.cheated)
+		if(playerState.second.cheated)
 		{
 			auto str = MetaString::createFromTextID("vcmi.broadcast.playerCheater");
-			str.replaceName(player.first);
+			str.replaceName(playerState.first);
 			broadcastSystemMessage(str);
 			playersCheated++;
 		}
@@ -626,7 +626,7 @@ void PlayerMessageProcessor::cheatMapReveal(PlayerColor player, bool reveal)
 
 void PlayerMessageProcessor::cheatPuzzleReveal(PlayerColor player)
 {
-	TeamState *t = gameHandler->gameState().getPlayerTeam(player);
+	const TeamState * t = gameHandler->gameState().getPlayerTeam(player);
 
 	for(auto & obj : gameHandler->gameState().getMap().getObjects<CGObelisk>())
 	{

server/queries/MapQueries.cpp

@@ -82,14 +82,13 @@ bool CGarrisonDialogQuery::blocksPack(const CPackForServer * pack) const
 	if(auto arts = dynamic_cast<const ExchangeArtifacts*>(pack))
 	{
 		auto id1 = arts->src.artHolder;
-		if(id1.hasValue())
-			if(!vstd::contains(ourIds, id1))
-				return true;
+		if(id1.hasValue() && !vstd::contains(ourIds, id1))
+			return true;
 
 		auto id2 = arts->dst.artHolder;
-		if(id2.hasValue())
-			if(!vstd::contains(ourIds, id2))
-				return true;
+		if(id2.hasValue() && !vstd::contains(ourIds, id2))
+			return true;
+
 		return false;
 	}
 	if(auto dismiss = dynamic_cast<const DisbandCreature*>(pack))

test/netpacks/NetPackFixture.cpp

@@ -24,8 +24,7 @@ NetPackFixture::~NetPackFixture() = default;
 
 void NetPackFixture::setUp()
 {
-    gameState = std::make_shared<GameStateFake>(nullptr);
-
+	gameState = std::make_shared<GameStateFake>(nullptr);
 }
 
 }