Merge pull request #1561 from rilian-la-te/modernize-serializer-spells

VCMI: modernize lib/serializer and modernize lib/spells

cmake_modules/VCMI_lib.cmake

@@ -125,6 +125,7 @@ macro(add_main_lib TARGET_NAME LIBRARY_TYPE)
 		${MAIN_LIB_DIR}/serializer/JsonSerializeFormat.cpp
 		${MAIN_LIB_DIR}/serializer/JsonSerializer.cpp
 		${MAIN_LIB_DIR}/serializer/JsonUpdater.cpp
+		${MAIN_LIB_DIR}/serializer/ILICReader.cpp
 
 		${MAIN_LIB_DIR}/spells/AbilityCaster.cpp
 		${MAIN_LIB_DIR}/spells/AdventureSpellMechanics.cpp
@@ -361,6 +362,7 @@ macro(add_main_lib TARGET_NAME LIBRARY_TYPE)
 		${MAIN_LIB_DIR}/serializer/JsonSerializeFormat.h
 		${MAIN_LIB_DIR}/serializer/JsonSerializer.h
 		${MAIN_LIB_DIR}/serializer/JsonUpdater.h
+		${MAIN_LIB_DIR}/serializer/ILICReader.h
 		${MAIN_LIB_DIR}/serializer/Cast.h
 
 		${MAIN_LIB_DIR}/spells/AbilityCaster.h

lib/serializer/BinaryDeserializer.cpp

@@ -24,13 +24,12 @@ CLoadFile::CLoadFile(const boost::filesystem::path & fname, int minimalVersion)
 	openNextFile(fname, minimalVersion);
 }
 
-CLoadFile::~CLoadFile()
-{
-}
+//must be instantiated in .cpp file for access to complete types of all member fields
+CLoadFile::~CLoadFile() = default;
 
 int CLoadFile::read(void * data, unsigned size)
 {
-	sfile->read((char*)data,size);
+	sfile->read(reinterpret_cast<char *>(data), size);
 	return size;
 }
 
@@ -51,7 +50,7 @@ void CLoadFile::openNextFile(const boost::filesystem::path & fname, int minimalV
 		//we can read
 		char buffer[4];
 		sfile->read(buffer, 4);
-		if(std::memcmp(buffer,"VCMI",4))
+		if(std::memcmp(buffer, "VCMI", 4) != 0)
 			THROW_FORMAT("Error: not a VCMI file(%s)!", fName);
 
 		serializer & serializer.fileVersion;
@@ -62,7 +61,7 @@ void CLoadFile::openNextFile(const boost::filesystem::path & fname, int minimalV
 		{
 			logGlobal->warn("Warning format version mismatch: found %d when current is %d! (file %s)\n", serializer.fileVersion, SERIALIZATION_VERSION , fName);
 
-			auto versionptr = (char*)&serializer.fileVersion;
+			auto * versionptr = reinterpret_cast<char *>(&serializer.fileVersion);
 			std::reverse(versionptr, versionptr + 4);
 			logGlobal->warn("Version number reversed is %x, checking...", serializer.fileVersion);
 
@@ -99,7 +98,7 @@ void CLoadFile::clear()
 void CLoadFile::checkMagicBytes(const std::string &text)
 {
 	std::string loaded = text;
-	read((void*)loaded.data(), (unsigned int)text.length());
+	read((void *)loaded.data(), static_cast<unsigned int>(text.length()));
 	if(loaded != text)
 		throw std::runtime_error("Magic bytes doesn't match!");
 }

lib/serializer/BinaryDeserializer.h

@@ -192,7 +192,7 @@ public:
 	void load(T &data)
 	{
 		unsigned length = sizeof(data);
-		char* dataPtr = (char*)&data;
+		char * dataPtr = reinterpret_cast<char *>(&data);
 		this->read(dataPtr,length);
 		if(reverseEndianess)
 			std::reverse(dataPtr, dataPtr + length);

lib/serializer/BinarySerializer.cpp

@@ -24,9 +24,8 @@ CSaveFile::CSaveFile(const boost::filesystem::path &fname)
 	openNextFile(fname);
 }
 
-CSaveFile::~CSaveFile()
-{
-}
+//must be instantiated in .cpp file for access to complete types of all member fields
+CSaveFile::~CSaveFile() = default;
 
 int CSaveFile::write(const void * data, unsigned size)
 {
@@ -73,7 +72,7 @@ void CSaveFile::clear()
 
 void CSaveFile::putMagicBytes(const std::string &text)
 {
-	write(text.c_str(), (unsigned int)text.length());
+	write(text.c_str(), static_cast<unsigned int>(text.length()));
 }
 
 VCMI_LIB_NAMESPACE_END

lib/serializer/CLoadIntegrityValidator.cpp

@@ -40,7 +40,7 @@ int CLoadIntegrityValidator::read( void * data, unsigned size )
 	if(!foundDesync)
 	{
 		controlFile->read(controlData.data(), size);
-		if(std::memcmp(data, controlData.data(), size))
+		if(std::memcmp(data, controlData.data(), size) != 0)
 		{
 			logGlobal->error("Desync found! Position: %d", primaryFile->sfile->tellg());
 			foundDesync = true;
@@ -57,7 +57,7 @@ std::unique_ptr<CLoadFile> CLoadIntegrityValidator::decay()
 	return std::move(primaryFile);
 }
 
-void CLoadIntegrityValidator::checkMagicBytes( const std::string &text )
+void CLoadIntegrityValidator::checkMagicBytes(const std::string & text) const
 {
 	assert(primaryFile);
 	assert(controlFile);

lib/serializer/CLoadIntegrityValidator.h

@@ -25,7 +25,7 @@ public:
 	CLoadIntegrityValidator(const boost::filesystem::path &primaryFileName, const boost::filesystem::path &controlFileName, int minimalVersion = SERIALIZATION_VERSION); //throws!
 
 	int read( void * data, unsigned size) override; //throws!
-	void checkMagicBytes(const std::string &text);
+	void checkMagicBytes(const std::string & text) const;
 
 	std::unique_ptr<CLoadFile> decay(); //returns primary file. CLoadIntegrityValidator stops being usable anymore
 };

lib/serializer/CMemorySerializer.cpp

@@ -32,13 +32,11 @@ int CMemorySerializer::write(const void * data, unsigned size)
 	return size;
 }
 
-CMemorySerializer::CMemorySerializer(): iser(this), oser(this)
+CMemorySerializer::CMemorySerializer(): iser(this), oser(this), readPos(0)
 {
-	readPos = 0;
 	registerTypes(iser);
 	registerTypes(oser);
 	iser.fileVersion = SERIALIZATION_VERSION;
 }
 
-
 VCMI_LIB_NAMESPACE_END

lib/serializer/CSerializer.cpp

@@ -17,17 +17,8 @@
 
 VCMI_LIB_NAMESPACE_BEGIN
 
-CSerializer::~CSerializer()
-{
-
-}
-
-CSerializer::CSerializer()
-{
-	smartVectorMembersSerialization = false;
-	sendStackInstanceByIds = false;
-}
-
+//must be instantiated in .cpp file for access to complete types of all member fields
+CSerializer::~CSerializer() = default;
 
 void CSerializer::addStdVecItems(CGameState *gs, LibClasses *lib)
 {

lib/serializer/CSerializer.h

@@ -80,10 +80,9 @@ class DLL_LINKAGE CSerializer
 	TTypeVecMap vectors; //entry must be a pointer to vector containing pointers to the objects of key type
 
 public:
-	bool smartVectorMembersSerialization;
-	bool sendStackInstanceByIds;
+	bool smartVectorMembersSerialization = false;
+	bool sendStackInstanceByIds = false;
 
-	CSerializer();
 	~CSerializer();
 
 	virtual void reportState(vstd::CLoggerBase * out){};

lib/serializer/CTypeList.cpp

@@ -74,7 +74,7 @@ std::vector<CTypeList::TypeInfoPtr> CTypeList::castSequence(TypeInfoPtr from, Ty
 		std::map<TypeInfoPtr, TypeInfoPtr> previous;
 		std::queue<TypeInfoPtr> q;
 		q.push(to);
-		while(q.size())
+		while(!q.empty())
 		{
 			auto typeNode = q.front();
 			q.pop();

lib/serializer/CTypeList.h

@@ -109,12 +109,7 @@ private:
 
 		return ptr;
 	}
-	CTypeList &operator=(CTypeList &)
-	{
-		// As above.
-		assert(0);
-		return *this;
-	}
+	CTypeList & operator=(CTypeList &) = delete;
 
 	TypeInfoPtr getTypeDescriptor(const std::type_info *type, bool throws = true) const; //if not throws, failure returns nullptr
 	TypeInfoPtr registerType(const std::type_info *type);

lib/serializer/Connection.cpp

@@ -21,16 +21,6 @@ VCMI_LIB_NAMESPACE_BEGIN
 using namespace boost;
 using namespace boost::asio::ip;
 
-#if defined(__hppa__) || \
-	defined(__m68k__) || defined(mc68000) || defined(_M_M68K) || \
-	(defined(__MIPS__) && defined(__MISPEB__)) || \
-	defined(__ppc__) || defined(__POWERPC__) || defined(_M_PPC) || \
-	defined(__sparc__)
-#define BIG_ENDIAN
-#else
-#define LIL_ENDIAN
-#endif
-
 struct ConnectionBuffers
 {
 	boost::asio::streambuf readBuffer;
@@ -58,7 +48,7 @@ void CConnection::init()
 	disableStackSendingByID();
 	registerTypes(iser);
 	registerTypes(oser);
-#ifdef LIL_ENDIAN
+#ifndef VCMI_ENDIAN_BIG
 	myEndianess = true;
 #else
 	myEndianess = false;
@@ -75,15 +65,21 @@ void CConnection::init()
 	iser.fileVersion = SERIALIZATION_VERSION;
 }
 
-CConnection::CConnection(std::string host, ui16 port, std::string Name, std::string UUID)
-	: io_service(std::make_shared<asio::io_service>()), iser(this), oser(this), name(Name), uuid(UUID), connectionID(0)
+CConnection::CConnection(const std::string & host, ui16 port, std::string Name, std::string UUID):
+	io_service(std::make_shared<asio::io_service>()),
+	iser(this),
+	oser(this),
+	name(std::move(Name)),
+	uuid(std::move(UUID))
 {
-	int i;
+	int i = 0;
 	boost::system::error_code error = asio::error::host_not_found;
 	socket = std::make_shared<tcp::socket>(*io_service);
 
 	tcp::resolver resolver(*io_service);
-	tcp::resolver::iterator end, pom, endpoint_iterator = resolver.resolve(tcp::resolver::query(host, std::to_string(port)),error);
+	tcp::resolver::iterator end;
+	tcp::resolver::iterator pom;
+	tcp::resolver::iterator endpoint_iterator = resolver.resolve(tcp::resolver::query(host, std::to_string(port)), error);
 	if(error)
 	{
 		logNetwork->error("Problem with resolving: \n%s", error.message());
@@ -97,7 +93,6 @@ CConnection::CConnection(std::string host, ui16 port, std::string Name, std::str
 		logNetwork->error("Critical problem: No endpoints found!");
 		goto connerror1;
 	}
-	i=0;
 	while(pom != end)
 	{
 		logNetwork->info("\t%d:%s", i, (boost::asio::ip::tcp::endpoint&)*pom);
@@ -129,13 +124,24 @@ connerror1:
 		logNetwork->error("No error info. ");
 	throw std::runtime_error("Can't establish connection :(");
 }
-CConnection::CConnection(std::shared_ptr<TSocket> Socket, std::string Name, std::string UUID)
-	: iser(this), oser(this), socket(Socket), name(Name), uuid(UUID), connectionID(0)
+CConnection::CConnection(std::shared_ptr<TSocket> Socket, std::string Name, std::string UUID):
+	iser(this),
+	oser(this),
+	socket(std::move(Socket)),
+	name(std::move(Name)),
+	uuid(std::move(UUID))
 {
 	init();
 }
-CConnection::CConnection(std::shared_ptr<TAcceptor> acceptor, std::shared_ptr<boost::asio::io_service> io_service, std::string Name, std::string UUID)
-	: io_service(io_service), iser(this), oser(this), name(Name), uuid(UUID), connectionID(0)
+CConnection::CConnection(const std::shared_ptr<TAcceptor> & acceptor,
+						 const std::shared_ptr<boost::asio::io_service> & io_service,
+						 std::string Name,
+						 std::string UUID):
+	io_service(io_service),
+	iser(this),
+	oser(this),
+	name(std::move(Name)),
+	uuid(std::move(UUID))
 {
 	boost::system::error_code error = asio::error::host_not_found;
 	socket = std::make_shared<tcp::socket>(*io_service);
@@ -181,8 +187,7 @@ int CConnection::write(const void * data, unsigned size)
 			return size;
 		}
 
-		int ret;
-		ret = static_cast<int>(asio::write(*socket,asio::const_buffers_1(asio::const_buffer(data,size))));
+		int ret = static_cast<int>(asio::write(*socket, asio::const_buffers_1(asio::const_buffer(data, size))));
 		return ret;
 	}
 	catch(...)

lib/serializer/Connection.h

@@ -88,8 +88,8 @@ public:
 	int connectionID;
 	std::shared_ptr<boost::thread> handler;
 
-	CConnection(std::string host, ui16 port, std::string Name, std::string UUID);
-	CConnection(std::shared_ptr<TAcceptor> acceptor, std::shared_ptr<boost::asio::io_service> Io_service, std::string Name, std::string UUID);
+	CConnection(const std::string & host, ui16 port, std::string Name, std::string UUID);
+	CConnection(const std::shared_ptr<TAcceptor> & acceptor, const std::shared_ptr<boost::asio::io_service> & Io_service, std::string Name, std::string UUID);
 	CConnection(std::shared_ptr<TSocket> Socket, std::string Name, std::string UUID); //use immediately after accepting connection into socket
 
 	void close();

lib/serializer/ILICReader.cpp

@@ -0,0 +1,49 @@
+/*
+ * JsonTreeSerializer.cpp, part of VCMI engine
+ *
+ * Authors: listed in file AUTHORS in main folder
+ *
+ * License: GNU General Public License v2.0 or later
+ * Full text of license available in license.txt file, in main folder
+ *
+ */
+#include "StdInc.h"
+#include "ILICReader.h"
+
+#include "../JsonNode.h"
+
+#include <vstd/StringUtils.h>
+
+VCMI_LIB_NAMESPACE_BEGIN
+
+void ILICReader::readLICPart(const JsonNode & part, const JsonSerializeFormat::TDecoder & decoder, bool val, std::vector<bool> & value) const
+{
+	for(const auto & index : part.Vector())
+	{
+		const std::string & identifier = index.String();
+		const std::string type = typeid(decltype(this)).name();
+
+		const si32 rawId = decoder(identifier);
+		if(rawId >= 0)
+		{
+			if(rawId < value.size())
+				value[rawId] = val;
+			else
+				logGlobal->error("%s::serializeLIC: id out of bounds %d", type, rawId);
+		}
+	}
+}
+
+void ILICReader::readLICPart(const JsonNode & part, const JsonSerializeFormat::TDecoder & decoder, std::set<si32> & value) const
+{
+	for(const auto & index : part.Vector())
+	{
+		const std::string & identifier = index.String();
+
+		const si32 rawId = decoder(identifier);
+		if(rawId != -1)
+			value.insert(rawId);
+	}
+}
+
+VCMI_LIB_NAMESPACE_END

lib/serializer/ILICReader.h

@@ -0,0 +1,23 @@
+/*
+ * ILICReader.h, part of VCMI engine
+ *
+ * Authors: listed in file AUTHORS in main folder
+ *
+ * License: GNU General Public License v2.0 or later
+ * Full text of license available in license.txt file, in main folder
+ *
+ */
+#pragma once
+
+#include "JsonTreeSerializer.h"
+
+VCMI_LIB_NAMESPACE_BEGIN
+
+class DLL_LINKAGE ILICReader
+{
+protected:
+	void readLICPart(const JsonNode & part, const JsonSerializeFormat::TDecoder & decoder, bool val, std::vector<bool> & value) const;
+	void readLICPart(const JsonNode & part, const JsonSerializeFormat::TDecoder & decoder, std::set<si32> & value) const;
+};
+
+VCMI_LIB_NAMESPACE_END

lib/serializer/JsonDeserializer.cpp

@@ -254,34 +254,4 @@ void JsonDeserializer::serializeRaw(const std::string & fieldName, JsonNode & va
 	}
 }
 
-void JsonDeserializer::readLICPart(const JsonNode & part, const TDecoder & decoder, const bool val, std::vector<bool> & value)
-{
-	for(size_t index = 0; index < part.Vector().size(); index++)
-	{
-		const std::string & identifier = part.Vector()[index].String();
-
-		const si32 rawId = decoder(identifier);
-		if(rawId >= 0)
-		{
-			if(rawId < value.size())
-				value[rawId] = val;
-			else
-				logGlobal->error("JsonDeserializer::serializeLIC: id out of bounds %d", rawId);
-		}
-	}
-}
-
-void JsonDeserializer::readLICPart(const JsonNode & part, const TDecoder & decoder, std::set<si32> & value)
-{
-	for(size_t index = 0; index < part.Vector().size(); index++)
-	{
-		const std::string & identifier = part.Vector()[index].String();
-
-		const si32 rawId = decoder(identifier);
-		if(rawId != -1)
-			value.insert(rawId);
-	}
-}
-
-
 VCMI_LIB_NAMESPACE_END

lib/serializer/JsonDeserializer.h

@@ -9,11 +9,12 @@
  */
 #pragma once
 
-#include  "JsonTreeSerializer.h"
+#include "ILICReader.h"
+#include "JsonTreeSerializer.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
-class DLL_LINKAGE JsonDeserializer: public JsonTreeSerializer<const JsonNode *>
+class DLL_LINKAGE JsonDeserializer: public JsonTreeSerializer<const JsonNode *>, public ILICReader
 {
 public:
 	JsonDeserializer(const IInstanceResolver * instanceResolver_, const JsonNode & root_);
@@ -35,10 +36,6 @@ protected:
 
 	void serializeInternal(std::string & value) override;
 	void serializeInternal(int64_t & value) override;
-
-private:
-	void readLICPart(const JsonNode & part, const TDecoder & decoder, const bool val, std::vector<bool> & value);
-	void readLICPart(const JsonNode & part, const TDecoder & decoder, std::set<si32> & value);
 };
 
 VCMI_LIB_NAMESPACE_END

lib/serializer/JsonSerializeFormat.cpp

@@ -15,9 +15,7 @@
 VCMI_LIB_NAMESPACE_BEGIN
 
 //JsonSerializeHelper
-JsonSerializeHelper::JsonSerializeHelper(JsonSerializeHelper && other):
-	owner(other.owner),
-	restoreState(false)
+JsonSerializeHelper::JsonSerializeHelper(JsonSerializeHelper && other) noexcept: owner(other.owner), restoreState(false)
 {
 	std::swap(restoreState, other.restoreState);
 }
@@ -40,33 +38,17 @@ JsonSerializeHelper::JsonSerializeHelper(JsonSerializeFormat * owner_)
 }
 
 //JsonStructSerializer
-JsonStructSerializer::JsonStructSerializer(JsonStructSerializer && other)
-	: JsonSerializeHelper(std::move(static_cast<JsonSerializeHelper &>(other)))
-{
-
-}
+JsonStructSerializer::JsonStructSerializer(JsonStructSerializer && other) noexcept: JsonSerializeHelper(std::move(static_cast<JsonSerializeHelper &>(other))) {}
 
 JsonStructSerializer::JsonStructSerializer(JsonSerializeFormat * owner_)
 	: JsonSerializeHelper(owner_)
 {
 }
 
-JsonStructSerializer::~JsonStructSerializer()
-{
-}
-
 //JsonArraySerializer
-JsonArraySerializer::JsonArraySerializer(JsonArraySerializer && other)
-	: JsonSerializeHelper(std::move(static_cast<JsonSerializeHelper &>(other)))
-{
-
-}
+JsonArraySerializer::JsonArraySerializer(JsonArraySerializer && other) noexcept: JsonSerializeHelper(std::move(static_cast<JsonSerializeHelper &>(other))) {}
 
-JsonArraySerializer::JsonArraySerializer(JsonSerializeFormat * owner_):
-	JsonSerializeHelper(owner_)
-{
-	thisNode = &owner->getCurrent();
-}
+JsonArraySerializer::JsonArraySerializer(JsonSerializeFormat * owner_): JsonSerializeHelper(owner_), thisNode(&owner->getCurrent()) {}
 
 JsonStructSerializer JsonArraySerializer::enterStruct(const size_t index)
 {
@@ -110,16 +92,20 @@ size_t JsonArraySerializer::size() const
 }
 
 //JsonSerializeFormat::LIC
-JsonSerializeFormat::LIC::LIC(const std::vector<bool> & Standard, const TDecoder Decoder, const TEncoder Encoder):
-	standard(Standard), decoder(Decoder), encoder(Encoder)
+JsonSerializeFormat::LIC::LIC(const std::vector<bool> & Standard, TDecoder Decoder, TEncoder Encoder):
+	standard(Standard),
+	decoder(std::move(Decoder)),
+	encoder(std::move(Encoder))
 {
 	any.resize(standard.size(), false);
 	all.resize(standard.size(), false);
 	none.resize(standard.size(), false);
 }
 
-JsonSerializeFormat::LICSet::LICSet(const std::set<si32>& Standard, const TDecoder Decoder, const TEncoder Encoder):
-	standard(Standard), decoder(Decoder), encoder(Encoder)
+JsonSerializeFormat::LICSet::LICSet(const std::set<si32> & Standard, TDecoder Decoder, TEncoder Encoder):
+	standard(Standard),
+	decoder(std::move(Decoder)),
+	encoder(std::move(Encoder))
 {
 
 }

lib/serializer/JsonSerializeFormat.h

@@ -28,7 +28,7 @@ public:
 class DLL_LINKAGE JsonSerializeHelper: public boost::noncopyable
 {
 public:
-	JsonSerializeHelper(JsonSerializeHelper && other);
+	JsonSerializeHelper(JsonSerializeHelper && other) noexcept;
 	virtual ~JsonSerializeHelper();
 
 	JsonSerializeFormat * operator->();
@@ -44,8 +44,8 @@ private:
 class DLL_LINKAGE JsonStructSerializer: public JsonSerializeHelper
 {
 public:
-	JsonStructSerializer(JsonStructSerializer && other);
-	~JsonStructSerializer();
+	JsonStructSerializer(JsonStructSerializer && other) noexcept;
+
 protected:
 	JsonStructSerializer(JsonSerializeFormat * owner_);
 
@@ -56,7 +56,7 @@ protected:
 class DLL_LINKAGE JsonArraySerializer: public JsonSerializeHelper
 {
 public:
-	JsonArraySerializer(JsonArraySerializer && other);
+	JsonArraySerializer(JsonArraySerializer && other) noexcept;
 
 	JsonStructSerializer enterStruct(const size_t index);
 	JsonArraySerializer enterArray(const size_t index);
@@ -102,17 +102,17 @@ class DLL_LINKAGE JsonSerializeFormat: public boost::noncopyable
 public:
 	///user-provided callback to resolve string identifier
 	///returns resolved identifier or -1 on error
-	typedef std::function<si32(const std::string &)> TDecoder;
+	using TDecoder = std::function<si32(const std::string &)>;
 
 	///user-provided callback to get string identifier
 	///may assume that object index is valid
-	typedef std::function<std::string(si32)> TEncoder;
+	using TEncoder = std::function<std::string(si32)>;
 
-	typedef std::function<void(JsonSerializeFormat &)> TSerialize;
+	using TSerialize = std::function<void(JsonSerializeFormat &)>;
 
 	struct LIC
 	{
-		LIC(const std::vector<bool> & Standard, const TDecoder Decoder, const TEncoder Encoder);
+		LIC(const std::vector<bool> & Standard, TDecoder Decoder, TEncoder Encoder);
 
 		const std::vector<bool> & standard;
 		const TDecoder decoder;
@@ -122,7 +122,7 @@ public:
 
 	struct LICSet
 	{
-		LICSet(const std::set<si32> & Standard, const TDecoder Decoder, const TEncoder Encoder);
+		LICSet(const std::set<si32> & Standard, TDecoder Decoder, TEncoder Encoder);
 
 		const std::set<si32> & standard;
 		const TDecoder decoder;

lib/serializer/JsonTreeSerializer.h

@@ -27,10 +27,9 @@ protected:
 	T currentObject;
 	std::vector<T> treeRoute;
 
-	JsonTreeSerializer(const IInstanceResolver * instanceResolver_, T root, const bool saving_, const bool updating_)
-		: JsonSerializeFormat(instanceResolver_, saving_, updating_),
-		currentObject(root),
-		treeRoute()
+	JsonTreeSerializer(const IInstanceResolver * instanceResolver_, T root, const bool saving_, const bool updating_):
+		JsonSerializeFormat(instanceResolver_, saving_, updating_),
+		currentObject(root)
 	{
 	}
 

lib/serializer/JsonUpdater.cpp

@@ -245,7 +245,7 @@ void JsonUpdater::serializeBonuses(const std::string & fieldName, CBonusSystemNo
 
 	if(toAdd.getType() == JsonNode::JsonType::DATA_VECTOR)
 	{
-		for(auto & item : toAdd.Vector())
+		for(const auto & item : toAdd.Vector())
 		{
 			auto b = JsonUtils::parseBonus(item);
 			value->addNewBonus(b);
@@ -256,7 +256,7 @@ void JsonUpdater::serializeBonuses(const std::string & fieldName, CBonusSystemNo
 
 	if(toRemove.getType() == JsonNode::JsonType::DATA_VECTOR)
 	{
-		for(auto & item : toRemove.Vector())
+		for(const auto & item : toRemove.Vector())
 		{
 			auto mask = JsonUtils::parseBonus(item);
 
@@ -280,34 +280,4 @@ void JsonUpdater::serializeBonuses(const std::string & fieldName, CBonusSystemNo
 	}
 }
 
-void JsonUpdater::readLICPart(const JsonNode & part, const TDecoder & decoder, const bool val, std::vector<bool> & value)
-{
-	for(size_t index = 0; index < part.Vector().size(); index++)
-	{
-		const std::string & identifier = part.Vector()[index].String();
-
-		const si32 rawId = decoder(identifier);
-		if(rawId >= 0)
-		{
-			if(rawId < value.size())
-				value[rawId] = val;
-			else
-				logGlobal->error("JsonUpdater::serializeLIC: id out of bounds %d", rawId);
-		}
-	}
-}
-
-void JsonUpdater::readLICPart(const JsonNode & part, const TDecoder & decoder, std::set<si32> & value)
-{
-	for(size_t index = 0; index < part.Vector().size(); index++)
-	{
-		const std::string & identifier = part.Vector()[index].String();
-
-		const si32 rawId = decoder(identifier);
-		if(rawId != -1)
-			value.insert(rawId);
-	}
-}
-
-
 VCMI_LIB_NAMESPACE_END

lib/serializer/JsonUpdater.h

@@ -9,13 +9,14 @@
  */
 #pragma once
 
-#include  "JsonTreeSerializer.h"
+#include "ILICReader.h"
+#include "JsonTreeSerializer.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
 class CBonusSystemNode;
 
-class DLL_LINKAGE JsonUpdater: public JsonTreeSerializer<const JsonNode *>
+class DLL_LINKAGE JsonUpdater: public JsonTreeSerializer<const JsonNode *>, public ILICReader
 {
 public:
 	JsonUpdater(const IInstanceResolver * instanceResolver_, const JsonNode & root_);
@@ -39,10 +40,6 @@ protected:
 
 	void serializeInternal(std::string & value) override;
 	void serializeInternal(int64_t & value) override;
-
-private:
-	void readLICPart(const JsonNode & part, const TDecoder & decoder, const bool val, std::vector<bool> & value);
-	void readLICPart(const JsonNode & part, const TDecoder & decoder, std::set<si32> & value);
 };
 
 VCMI_LIB_NAMESPACE_END

lib/spells/AdventureSpellMechanics.cpp

@@ -79,7 +79,7 @@ ESpellCastResult AdventureSpellMechanics::applyAdventureEffects(SpellCastEnviron
 
 		owner->getEffects(bonuses, schoolLevel, false, parameters.caster->getEnchantPower(owner));
 
-		for(Bonus b : bonuses)
+		for(const Bonus & b : bonuses)
 		{
 			GiveBonus gb;
 			gb.id = parameters.caster->id.getNum();
@@ -182,7 +182,7 @@ ESpellCastResult SummonBoatMechanics::applyAdventureEffects(SpellCastEnvironment
 	{
 		if(obj && obj->ID == Obj::BOAT)
 		{
-			const CGBoat *b = static_cast<const CGBoat*>(obj);
+			const auto * b = dynamic_cast<const CGBoat *>(obj);
 			if(b->hero)
 				continue; //we're looking for unoccupied boat
 
@@ -249,7 +249,7 @@ ESpellCastResult ScuttleBoatMechanics::applyAdventureEffects(SpellCastEnvironmen
 
 	//TODO: test range, visibility
 	const TerrainTile *t = &env->getMap()->getTile(parameters.pos);
-	if(!t->visitableObjects.size() || t->visitableObjects.back()->ID != Obj::BOAT)
+	if(t->visitableObjects.empty() || t->visitableObjects.back()->ID != Obj::BOAT)
 	{
 		env->complain("There is no boat to scuttle!");
 		return ESpellCastResult::ERROR;
@@ -328,7 +328,7 @@ ESpellCastResult DimensionDoorMechanics::applyAdventureEffects(SpellCastEnvironm
 	{
 		SetMovePoints smp;
 		smp.hid = parameters.caster->id;
-		if(movementCost < (int)parameters.caster->movement)
+		if(movementCost < static_cast<int>(parameters.caster->movement))
 			smp.val = parameters.caster->movement - movementCost;
 		else
 			smp.val = 0;
@@ -356,7 +356,7 @@ ESpellCastResult TownPortalMechanics::applyAdventureEffects(SpellCastEnvironment
 		if(nullptr == destination)
 			return ESpellCastResult::ERROR;
 
-		if((int)parameters.caster->movement < moveCost)
+		if(static_cast<int>(parameters.caster->movement) < moveCost)
 			return ESpellCastResult::ERROR;
 
 		if(destination->visitingHero)
@@ -372,7 +372,7 @@ ESpellCastResult TownPortalMechanics::applyAdventureEffects(SpellCastEnvironment
 	{
 		const TerrainTile & tile = env->getMap()->getTile(parameters.pos);
 
-		const auto topObj = tile.topVisitableObj(false);
+		auto * const topObj = tile.topVisitableObj(false);
 
 		if(!topObj)
 		{
@@ -406,7 +406,7 @@ ESpellCastResult TownPortalMechanics::applyAdventureEffects(SpellCastEnvironment
 			return ESpellCastResult::ERROR;
 		}
 
-		if((int)parameters.caster->movement < moveCost)
+		if(static_cast<int>(parameters.caster->movement) < moveCost)
 		{
 			env->complain("This hero has not enough movement points!");
 			return ESpellCastResult::ERROR;
@@ -449,7 +449,7 @@ ESpellCastResult TownPortalMechanics::beginCast(SpellCastEnvironment * env, cons
 
 	const int moveCost = movementCost(parameters);
 
-	if((int)parameters.caster->movement < moveCost)
+	if(static_cast<int>(parameters.caster->movement) < moveCost)
 	{
 		InfoWindow iw;
 		iw.player = parameters.caster->tempOwner;
@@ -464,7 +464,7 @@ ESpellCastResult TownPortalMechanics::beginCast(SpellCastEnvironment * env, cons
 		{
 			if(reply.getType() == JsonNode::JsonType::DATA_INTEGER)
 			{
-				ObjectInstanceID townId((si32)reply.Integer());
+				ObjectInstanceID townId(static_cast<si32>(reply.Integer()));
 
 				const CGObjectInstance * o = env->getCb()->getObj(townId, true);
 				if(o == nullptr)
@@ -488,7 +488,7 @@ ESpellCastResult TownPortalMechanics::beginCast(SpellCastEnvironment * env, cons
 
 		MapObjectSelectDialog request;
 
-		for(auto t : towns)
+		for(const auto * t : towns)
 		{
 			if(t->visitingHero == nullptr) //empty town
 				request.objects.push_back(t->id);

lib/spells/BattleSpellMechanics.cpp

@@ -127,11 +127,12 @@ namespace SRSLPraserHelpers
 	}
 }
 
-
-BattleSpellMechanics::BattleSpellMechanics(const IBattleCast * event, std::shared_ptr<effects::Effects> effects_, std::shared_ptr<IReceptiveCheck> targetCondition_)
-	: BaseMechanics(event),
-	effects(effects_),
-	targetCondition(targetCondition_)
+BattleSpellMechanics::BattleSpellMechanics(const IBattleCast * event,
+										   std::shared_ptr<effects::Effects> effects_,
+										   std::shared_ptr<IReceptiveCheck> targetCondition_):
+	BaseMechanics(event),
+	effects(std::move(effects_)),
+	targetCondition(std::move(targetCondition_))
 {}
 
 BattleSpellMechanics::~BattleSpellMechanics() = default;
@@ -167,7 +168,7 @@ bool BattleSpellMechanics::canBeCast(Problem & problem) const
 	{
 	case Mode::HERO:
 		{
-			const CGHeroInstance * castingHero = dynamic_cast<const CGHeroInstance *>(caster);//todo: unify hero|creature spell cost
+			const auto * castingHero = dynamic_cast<const CGHeroInstance *>(caster); //todo: unify hero|creature spell cost
 			if(!castingHero)
 			{
 				logGlobal->debug("CSpell::canBeCast: invalid caster");
@@ -272,7 +273,7 @@ void BattleSpellMechanics::cast(ServerCallback * server, const Target & target)
 	//calculate spell cost
 	if(mode == Mode::HERO)
 	{
-		auto casterHero = dynamic_cast<const CGHeroInstance *>(caster);
+		const auto * casterHero = dynamic_cast<const CGHeroInstance *>(caster);
 		spellCost = battle()->battleGetSpellCost(owner, casterHero);
 
 		if(nullptr != otherHero) //handle mana channel
@@ -383,7 +384,7 @@ void BattleSpellMechanics::beforeCast(BattleSpellCast & sc, vstd::RNG & rng, con
 	std::set<const battle::Unit *> unitTargets = collectTargets();
 
 	//process them
-	for(auto unit : unitTargets)
+	for(const auto * unit : unitTargets)
 		filterUnit(unit);
 
 	//and update targets
@@ -405,7 +406,7 @@ void BattleSpellMechanics::beforeCast(BattleSpellCast & sc, vstd::RNG & rng, con
 		}
 	}
 
-	for(auto unit : resisted)
+	for(const auto * unit : resisted)
 		sc.resistedCres.insert(unit->unitId());
 }
 
@@ -448,16 +449,16 @@ void BattleSpellMechanics::doRemoveEffects(ServerCallback * server, const std::v
 {
 	SetStackEffect sse;
 
-	for(auto unit : targets)
+	for(const auto * unit : targets)
 	{
 		std::vector<Bonus> buffer;
 		auto bl = unit->getBonuses(selector);
 
-		for(auto item : *bl)
+		for(const auto & item : *bl)
 			buffer.emplace_back(*item);
 
 		if(!buffer.empty())
-			sse.toRemove.push_back(std::make_pair(unit->unitId(), buffer));
+			sse.toRemove.emplace_back(unit->unitId(), buffer);
 	}
 
 	if(!sse.toRemove.empty())
@@ -487,8 +488,10 @@ std::set<BattleHex> BattleSpellMechanics::spellRangeInHexes(BattleHex centralHex
 
 	if(rng.size() >= 2 && rng[0] != 'X') //there is at least one hex in range (+artificial comma)
 	{
-		std::string number1, number2;
-		int beg, end;
+		std::string number1;
+		std::string number2;
+		int beg = 0;
+		int end = 0;
 		bool readingFirst = true;
 		for(auto & elem : rng)
 		{
@@ -504,12 +507,12 @@ std::set<BattleHex> BattleSpellMechanics::spellRangeInHexes(BattleHex centralHex
 				//calculating variables
 				if(readingFirst)
 				{
-					beg = atoi(number1.c_str());
+					beg = std::stoi(number1);
 					number1 = "";
 				}
 				else
 				{
-					end = atoi(number2.c_str());
+					end = std::stoi(number2);
 					number2 = "";
 				}
 				//obtaining new hexes
@@ -524,7 +527,7 @@ std::set<BattleHex> BattleSpellMechanics::spellRangeInHexes(BattleHex centralHex
 					readingFirst = true;
 				}
 				//adding obtained hexes
-				for(auto & curLayer_it : curLayer)
+				for(const auto & curLayer_it : curLayer)
 				{
 					ret.insert(curLayer_it);
 				}
@@ -532,7 +535,7 @@ std::set<BattleHex> BattleSpellMechanics::spellRangeInHexes(BattleHex centralHex
 			}
 			else if(elem == '-') //dash
 			{
-				beg = atoi(number1.c_str());
+				beg = std::stoi(number1);
 				number1 = "";
 				readingFirst = false;
 			}
@@ -546,7 +549,7 @@ Target BattleSpellMechanics::transformSpellTarget(const Target & aimPoint) const
 {
 	Target spellTarget;
 
-	if(aimPoint.size() < 1)
+	if(aimPoint.empty())
 	{
 		logGlobal->error("Aimed spell cast with no destination.");
 	}
@@ -560,7 +563,7 @@ Target BattleSpellMechanics::transformSpellTarget(const Target & aimPoint) const
 		if(aimPointHex.isValid())
 		{
 			auto spellRange = spellRangeInHexes(aimPointHex);
-			for(auto & hex : spellRange)
+			for(const auto & hex : spellRange)
 				spellTarget.push_back(Destination(hex));
 		}
 	}

lib/spells/BonusCaster.cpp

@@ -22,10 +22,10 @@ VCMI_LIB_NAMESPACE_BEGIN
 namespace spells
 {
 
-BonusCaster::BonusCaster(const Caster * actualCaster_, std::shared_ptr<Bonus> bonus_)
-	: ProxyCaster(actualCaster_),
+BonusCaster::BonusCaster(const Caster * actualCaster_, std::shared_ptr<Bonus> bonus_):
+	ProxyCaster(actualCaster_),
 	actualCaster(actualCaster_),
-	bonus(bonus_)
+	bonus(std::move(bonus_))
 {
 
 }

lib/spells/CSpellHandler.cpp

@@ -84,24 +84,6 @@ static const ESpellSchool SCHOOL_ORDER[4] =
 };
 } //namespace SpellConfig
 
-///CSpell::LevelInfo
-CSpell::LevelInfo::LevelInfo():
-	cost(0),
-	power(0),
-	AIValue(0),
-	smartTarget(true),
-	clearTarget(false),
-	clearAffected(false),
-	range("0")
-{
-
-}
-
-CSpell::LevelInfo::~LevelInfo()
-{
-
-}
-
 ///CSpell
 CSpell::CSpell():
 	id(SpellID::NONE),
@@ -115,23 +97,19 @@ CSpell::CSpell():
 	damage(false),
 	offensive(false),
 	special(true),
-	targetType(spells::AimType::NO_TARGET),
-	mechanics(),
-	adventureMechanics()
+	targetType(spells::AimType::NO_TARGET)
 {
 	levels.resize(GameConstants::SPELL_SCHOOL_LEVELS);
 }
 
-CSpell::~CSpell()
-{
-
-}
+//must be instantiated in .cpp file for access to complete types of all member fields
+CSpell::~CSpell() = default;
 
 bool CSpell::adventureCast(SpellCastEnvironment * env, const AdventureSpellCastParameters & parameters) const
 {
 	assert(env);
 
-	if(!adventureMechanics.get())
+	if(!adventureMechanics)
 	{
 		env->complain("Invalid adventure spell cast attempt!");
 		return false;
@@ -185,7 +163,7 @@ void CSpell::forEachSchool(const std::function<void(const spells::SchoolInfo &,
 	bool stop = false;
 	for(ESpellSchool iter : SpellConfig::SCHOOL_ORDER)
 	{
-		const spells::SchoolInfo & cnf = SpellConfig::SCHOOL[(ui8)iter];
+		const spells::SchoolInfo & cnf = SpellConfig::SCHOOL[static_cast<ui8>(iter)];
 		if(school.at(cnf.id))
 		{
 			cb(cnf, stop);
@@ -405,15 +383,15 @@ int64_t CSpell::adjustRawDamage(const spells::Caster * caster, const battle::Uni
 	//affected creature-specific part
 	if(nullptr != affectedCreature)
 	{
-		auto bearer = affectedCreature;
+		const auto * bearer = affectedCreature;
 		//applying protections - when spell has more then one elements, only one protection should be applied (I think)
 		forEachSchool([&](const spells::SchoolInfo & cnf, bool & stop)
 		{
-			if(bearer->hasBonusOfType(Bonus::SPELL_DAMAGE_REDUCTION, (ui8)cnf.id))
+			if(bearer->hasBonusOfType(Bonus::SPELL_DAMAGE_REDUCTION, static_cast<ui8>(cnf.id)))
 			{
-				ret *= 100 - bearer->valOfBonuses(Bonus::SPELL_DAMAGE_REDUCTION, (ui8)cnf.id);
+				ret *= 100 - bearer->valOfBonuses(Bonus::SPELL_DAMAGE_REDUCTION, static_cast<ui8>(cnf.id));
 				ret /= 100;
-				stop = true;//only bonus from one school is used
+				stop = true; //only bonus from one school is used
 			}
 		});
 
@@ -439,7 +417,7 @@ int64_t CSpell::adjustRawDamage(const spells::Caster * caster, const battle::Uni
 
 int64_t CSpell::calculateRawEffectValue(int32_t effectLevel, int32_t basePowerMultiplier, int32_t levelPowerMultiplier) const
 {
-	return (int64_t)basePowerMultiplier * getBasePower() + levelPowerMultiplier * getLevelPower(effectLevel);
+	return static_cast<int64_t>(basePowerMultiplier) * getBasePower() + levelPowerMultiplier * getLevelPower(effectLevel);
 }
 
 void CSpell::setIsOffensive(const bool val)
@@ -540,18 +518,7 @@ CSpell::AnimationItem::AnimationItem() :
 
 }
 
-
 ///CSpell::AnimationInfo
-CSpell::AnimationInfo::AnimationInfo()
-{
-
-}
-
-CSpell::AnimationInfo::~AnimationInfo()
-{
-
-}
-
 std::string CSpell::AnimationInfo::selectProjectile(const double angle) const
 {
 	std::string res;
@@ -577,7 +544,7 @@ CSpell::TargetInfo::TargetInfo(const CSpell * spell, const int level, spells::Mo
 	clearAffected(false),
 	clearTarget(false)
 {
-	auto & levelInfo = spell->getLevelInfo(level);
+	const auto & levelInfo = spell->getLevelInfo(level);
 
 	smart = levelInfo.smartTarget;
 	massive = levelInfo.range == "X";
@@ -592,10 +559,6 @@ bool DLL_LINKAGE isInScreenRange(const int3 & center, const int3 & pos)
 }
 
 ///CSpellHandler
-CSpellHandler::CSpellHandler() = default;
-
-CSpellHandler::~CSpellHandler() = default;
-
 std::vector<JsonNode> CSpellHandler::loadLegacyData(size_t dataSize)
 {
 	using namespace SpellConfig;
@@ -648,8 +611,8 @@ std::vector<JsonNode> CSpellHandler::loadLegacyData(size_t dataSize)
 
 			auto & chances = lineNode["gainChance"].Struct();
 
-			for(size_t i = 0; i < GameConstants::F_NUMBER; i++)
-				chances[ETownType::names[i]].Integer() = static_cast<si64>(parser.readNumber());
+			for(const auto & name : ETownType::names)
+				chances[name].Integer() = static_cast<si64>(parser.readNumber());
 
 			auto AIVals = parser.readNumArray<si32>(GameConstants::SPELL_SCHOOL_LEVELS);
 
@@ -713,7 +676,7 @@ CSpell * CSpellHandler::loadFromJson(const std::string & scope, const JsonNode &
 
 	SpellID id(static_cast<si32>(index));
 
-	CSpell * spell = new CSpell();
+	auto * spell = new CSpell();
 	spell->id = id;
 	spell->identifier = identifier;
 	spell->modScope = scope;
@@ -774,9 +737,9 @@ CSpell * CSpellHandler::loadFromJson(const std::string & scope, const JsonNode &
 	{
 		if(counteredSpell.second.Bool())
 		{
-			VLC->modh->identifiers.requestIdentifier(counteredSpell.second.meta, counteredSpell.first, [=](si32 id)
+			VLC->modh->identifiers.requestIdentifier(counteredSpell.second.meta, counteredSpell.first, [=](si32 id) 
 			{
-				spell->counteredSpells.push_back(SpellID(id));
+				spell->counteredSpells.emplace_back(id);
 			});
 		}
 	}
@@ -820,7 +783,7 @@ CSpell * CSpellHandler::loadFromJson(const std::string & scope, const JsonNode &
 
 	spell->special = flags["special"].Bool();
 
-	auto findBonus = [&](std::string name, std::vector<Bonus::BonusType> & vec)
+	auto findBonus = [&](const std::string & name, std::vector<Bonus::BonusType> & vec)
 	{
 		auto it = bonusNameMap.find(name);
 		if(it == bonusNameMap.end())
@@ -829,11 +792,11 @@ CSpell * CSpellHandler::loadFromJson(const std::string & scope, const JsonNode &
 		}
 		else
 		{
-			vec.push_back((Bonus::BonusType)it->second);
+			vec.push_back(static_cast<Bonus::BonusType>(it->second));
 		}
 	};
 
-	auto readBonusStruct = [&](std::string name, std::vector<Bonus::BonusType> & vec)
+	auto readBonusStruct = [&](const std::string & name, std::vector<Bonus::BonusType> & vec)
 	{
 		for(auto bonusData: json[name].Struct())
 		{

lib/spells/CSpellHandler.h

@@ -97,9 +97,6 @@ public:
 
 	struct DLL_LINKAGE AnimationInfo
 	{
-		AnimationInfo();
-		~AnimationInfo();
-
 		///displayed on all affected targets.
 		TAnimationQueue affect;
 
@@ -123,17 +120,18 @@ public:
 
 		std::string selectProjectile(const double angle) const;
 	} animationInfo;
+
 public:
 	struct LevelInfo
 	{
-		si32 cost;
-		si32 power;
-		si32 AIValue;
+		si32 cost = 0;
+		si32 power = 0;
+		si32 AIValue = 0;
 
-		bool smartTarget;
-		bool clearTarget;
-		bool clearAffected;
-		std::string range;
+		bool smartTarget = true;
+		bool clearTarget = false;
+		bool clearAffected = false;
+		std::string range = "0";
 
 		//TODO: remove these two when AI will understand special effects
 		std::vector<std::shared_ptr<Bonus>> effects; //deprecated
@@ -141,9 +139,6 @@ public:
 
 		JsonNode battleEffects;
 
-		LevelInfo();
-		~LevelInfo();
-
 		template <typename Handler> void serialize(Handler & h, const int version)
 		{
 			h & cost;
@@ -369,9 +364,6 @@ bool DLL_LINKAGE isInScreenRange(const int3 &center, const int3 &pos); //for spe
 class DLL_LINKAGE CSpellHandler: public CHandlerBase<SpellID, spells::Spell, CSpell, spells::Service>
 {
 public:
-	CSpellHandler();
-	virtual ~CSpellHandler();
-
 	///IHandler base
 	std::vector<JsonNode> loadLegacyData(size_t dataSize) override;
 	void afterLoadFinalization() override;

lib/spells/ISpellMechanics.cpp

@@ -62,7 +62,7 @@ class CustomMechanicsFactory : public ISpellMechanicsFactory
 public:
 	std::unique_ptr<Mechanics> create(const IBattleCast * event) const override
 	{
-		BattleSpellMechanics * ret = new BattleSpellMechanics(event, effects, targetCondition);
+		auto * ret = new BattleSpellMechanics(event, effects, targetCondition);
 		return std::unique_ptr<Mechanics>(ret);
 	}
 protected:
@@ -117,7 +117,7 @@ public:
 
 			if(!levelInfo.effects.empty())
 			{
-				auto timed = new effects::Timed();
+				auto * timed = new effects::Timed();
 				timed->cumulative = false;
 				timed->bonus = levelInfo.effects;
 				effect.reset(timed);
@@ -125,7 +125,7 @@ public:
 
 			if(!levelInfo.cumulativeEffects.empty())
 			{
-				auto timed = new effects::Timed();
+				auto * timed = new effects::Timed();
 				timed->cumulative = true;
 				timed->bonus = levelInfo.cumulativeEffects;
 				effect.reset(timed);
@@ -137,20 +137,15 @@ public:
 	}
 };
 
-
-BattleCast::BattleCast(const CBattleInfoCallback * cb_, const Caster * caster_, const Mode mode_, const CSpell * spell_)
-	: spell(spell_),
+BattleCast::BattleCast(const CBattleInfoCallback * cb_, const Caster * caster_, const Mode mode_, const CSpell * spell_):
+	spell(spell_),
 	cb(cb_),
+	gameCb(IObjectInterface::cb), //FIXME: pass player callback (problem is that BattleAI do not have one)
 	caster(caster_),
 	mode(mode_),
-	magicSkillLevel(),
-	effectPower(),
-	effectDuration(),
-	effectValue(),
 	smart(boost::logic::indeterminate),
 	massive(boost::logic::indeterminate)
 {
-	gameCb = IObjectInterface::cb; //FIXME: pass player callback (problem is that BattleAI do not have one)
 }
 
 BattleCast::BattleCast(const BattleCast & orig, const Caster * caster_)
@@ -245,7 +240,7 @@ void BattleCast::setEffectValue(BattleCast::Value64 value)
 	effectValue = boost::make_optional(value);
 }
 
-void BattleCast::applyEffects(ServerCallback * server, Target target,  bool indirect, bool ignoreImmunity) const
+void BattleCast::applyEffects(ServerCallback * server, const Target & target, bool indirect, bool ignoreImmunity) const
 {
 	auto m = spell->battleMechanics(this);
 
@@ -296,7 +291,7 @@ void BattleCast::cast(ServerCallback * server, Target target)
 
 			if(!mirrorTargets.empty())
 			{
-				auto mirrorDestination = (*RandomGeneratorUtil::nextItem(mirrorTargets, *server->getRNG()));
+				const auto * mirrorDestination = (*RandomGeneratorUtil::nextItem(mirrorTargets, *server->getRNG()));
 
 				Target mirrorTarget;
 				mirrorTarget.emplace_back(mirrorDestination);
@@ -325,7 +320,7 @@ bool BattleCast::castIfPossible(ServerCallback * server, Target target)
 {
 	if(spell->canBeCast(cb, mode, caster))
 	{
-		cast(server, target);
+		cast(server, std::move(target));
 		return true;
 	}
 	return false;
@@ -397,10 +392,8 @@ ISpellMechanicsFactory::ISpellMechanicsFactory(const CSpell * s)
 
 }
 
-ISpellMechanicsFactory::~ISpellMechanicsFactory()
-{
-
-}
+//must be instantiated in .cpp file for access to complete types of all member fields
+ISpellMechanicsFactory::~ISpellMechanicsFactory() = default;
 
 std::unique_ptr<ISpellMechanicsFactory> ISpellMechanicsFactory::get(const CSpell * s)
 {
@@ -420,16 +413,14 @@ Mechanics::Mechanics()
 
 Mechanics::~Mechanics() = default;
 
-BaseMechanics::BaseMechanics(const IBattleCast * event)
-	: Mechanics(),
+BaseMechanics::BaseMechanics(const IBattleCast * event):
 	owner(event->getSpell()),
 	mode(event->getMode()),
 	smart(event->isSmart()),
-	massive(event->isMassive())
+	massive(event->isMassive()),
+	cb(event->getBattle()),
+	gameCb(event->getGame())
 {
-	cb = event->getBattle();
-	gameCb = event->getGame();
-
 	caster = event->getCaster();
 
 	//FIXME: do not crash on invalid side
@@ -510,7 +501,7 @@ bool BaseMechanics::adaptProblem(ESpellCastProblem::ESpellCastProblem source, Pr
 		{
 			MetaString text;
 			//TODO: refactor
-			auto hero = dynamic_cast<const CGHeroInstance *>(caster);
+			const auto * hero = dynamic_cast<const CGHeroInstance *>(caster);
 			if(!hero)
 				return adaptGenericProblem(target);
 

lib/spells/ISpellMechanics.h

@@ -49,7 +49,7 @@ namespace scripting
 class DLL_LINKAGE SpellCastEnvironment : public ServerCallback
 {
 public:
-	virtual ~SpellCastEnvironment(){};
+	virtual ~SpellCastEnvironment() = default;
 
 	virtual const CMap * getMap() const = 0;
 	virtual const CGameInfoCallback * getCb() const = 0;
@@ -128,7 +128,7 @@ public:
 	void setEffectValue(Value64 value);
 
 	///only apply effects to specified targets
-	void applyEffects(ServerCallback * server, Target target, bool indirect = false, bool ignoreImmunity = false) const;
+	void applyEffects(ServerCallback * server, const Target & target, bool indirect = false, bool ignoreImmunity = false) const;
 
 	///normal cast
 	void cast(ServerCallback * server, Target target);

lib/spells/Problem.cpp

@@ -18,19 +18,9 @@ namespace spells
 namespace detail
 {
 
-ProblemImpl::ProblemImpl()
-{
-
-}
-
-ProblemImpl::~ProblemImpl()
-{
-
-}
-
 void ProblemImpl::add(MetaString && description, Severity severity)
 {
-	data.push_back(std::make_pair(description, severity));
+	data.emplace_back(description, severity);
 }
 
 void ProblemImpl::getAll(std::vector<std::string> & target) const

lib/spells/Problem.h

@@ -24,9 +24,6 @@ namespace detail
 class DLL_LINKAGE ProblemImpl : public Problem
 {
 public:
-	ProblemImpl();
-	virtual ~ProblemImpl();
-
 	void add(MetaString && description, Severity severity = CRITICAL) override;
 
 	void getAll(std::vector<std::string> & target) const override;

lib/spells/TargetCondition.cpp

@@ -26,20 +26,11 @@ VCMI_LIB_NAMESPACE_BEGIN
 namespace spells
 {
 
-TargetConditionItem::TargetConditionItem() = default;
-TargetConditionItem::~TargetConditionItem() = default;
-
 class TargetConditionItemBase : public TargetConditionItem
 {
 public:
-	bool inverted;
-	bool exclusive;
-
-	TargetConditionItemBase()
-		: inverted(false),
-		exclusive(false)
-	{
-	}
+	bool inverted = false;
+	bool exclusive = false;
 
 	void setInverted(bool value) override
 	{
@@ -87,10 +78,7 @@ private:
 class CreatureCondition : public TargetConditionItemBase
 {
 public:
-	CreatureCondition(CreatureID type_)
-		: type(type_)
-	{
-	}
+	CreatureCondition(const CreatureID & type_): type(type_) {}
 
 protected:
 	bool check(const Mechanics * m, const battle::Unit * target) const override
@@ -218,11 +206,6 @@ protected:
 //for Hypnotize
 class HealthValueCondition : public TargetConditionItemBase
 {
-public:
-	HealthValueCondition()
-	{
-	}
-
 protected:
 	bool check(const Mechanics * m, const battle::Unit * target) const override
 	{
@@ -238,8 +221,7 @@ protected:
 class SpellEffectCondition : public TargetConditionItemBase
 {
 public:
-	SpellEffectCondition(SpellID spellID_)
-		: spellID(spellID_)
+	SpellEffectCondition(const SpellID & spellID_): spellID(spellID_)
 	{
 		std::stringstream builder;
 		builder << "source_" << Bonus::SPELL_EFFECT << "id_" << spellID.num;
@@ -262,13 +244,6 @@ private:
 
 class ReceptiveFeatureCondition : public TargetConditionItemBase
 {
-public:
-	ReceptiveFeatureCondition()
-	{
-		selector = Selector::type()(Bonus::RECEPTIVE);
-		cachingString = "type_RECEPTIVE";
-	}
-
 protected:
 	bool check(const Mechanics * m, const battle::Unit * target) const override
 	{
@@ -276,17 +251,12 @@ protected:
 	}
 
 private:
-	CSelector selector;
-	std::string cachingString;
+	CSelector selector = Selector::type()(Bonus::RECEPTIVE);
+	std::string cachingString = "type_RECEPTIVE";
 };
 
 class ImmunityNegationCondition : public TargetConditionItemBase
 {
-public:
-	ImmunityNegationCondition()
-	{
-	}
-
 protected:
 	bool check(const Mechanics * m, const battle::Unit * target) const override
 	{
@@ -404,16 +374,12 @@ const TargetConditionItemFactory * TargetConditionItemFactory::getDefault()
 	return singleton.get();
 }
 
-TargetCondition::TargetCondition() = default;
-
-TargetCondition::~TargetCondition() = default;
-
 bool TargetCondition::isReceptive(const Mechanics * m, const battle::Unit * target) const
 {
 	if(!check(absolute, m, target))
 		return false;
 
-	for(auto item : negation)
+	for(const auto & item : negation)
 	{
 		if(item->isReceptive(m, target))
 			return true;
@@ -462,7 +428,7 @@ bool TargetCondition::check(const ItemVector & condition, const Mechanics * m, c
 	bool nonExclusiveCheck = false;
 	bool nonExclusiveExits = false;
 
-	for(auto & item : condition)
+	for(const auto & item : condition)
 	{
 		if(item->isExclusive())
 		{
@@ -481,7 +447,7 @@ bool TargetCondition::check(const ItemVector & condition, const Mechanics * m, c
 
 void TargetCondition::loadConditions(const JsonNode & source, bool exclusive, bool inverted, const ItemFactory * itemFactory)
 {
-	for(auto & keyValue : source.Struct())
+	for(const auto & keyValue : source.Struct())
 	{
 		bool isAbsolute;
 
@@ -494,9 +460,11 @@ void TargetCondition::loadConditions(const JsonNode & source, bool exclusive, bo
 		else
 			continue;
 
-		std::string scope, type, identifier;
+		std::string scope;
+		std::string type;
+		std::string identifier;
 
-		VLC->modh->parseIdentifier(keyValue.first, scope, type, identifier);
+		CModHandler::parseIdentifier(keyValue.first, scope, type, identifier);
 
 		std::shared_ptr<TargetConditionItem> item = itemFactory->createConfigurable(scope, type, identifier);
 

lib/spells/TargetCondition.h

@@ -30,9 +30,6 @@ class Mechanics;
 class DLL_LINKAGE TargetConditionItem : public IReceptiveCheck
 {
 public:
-	TargetConditionItem();
-	virtual ~TargetConditionItem();
-
 	virtual void setInverted(bool value) = 0;
 	virtual void setExclusive(bool value) = 0;
 
@@ -70,9 +67,6 @@ public:
 	ItemVector absolute;
 	ItemVector negation;
 
-	TargetCondition();
-	virtual ~TargetCondition();
-
 	bool isReceptive(const Mechanics * m, const battle::Unit * target) const override;
 
 	void serializeJson(JsonSerializeFormat & handler, const ItemFactory * itemFactory);

lib/spells/ViewSpellInt.cpp

@@ -16,12 +16,6 @@
 
 VCMI_LIB_NAMESPACE_BEGIN
 
-ObjectPosInfo::ObjectPosInfo():
-	pos(), id(Obj::NO_OBJ), subId(-1), owner(PlayerColor::CANNOT_DETERMINE)
-{
-
-}
-
 ObjectPosInfo::ObjectPosInfo(const CGObjectInstance * obj):
 	pos(obj->visitablePos()), id(obj->ID), subId(obj->subID), owner(obj->tempOwner)
 {

lib/spells/ViewSpellInt.h

@@ -20,11 +20,11 @@ VCMI_LIB_NAMESPACE_BEGIN
  struct DLL_LINKAGE ObjectPosInfo
  {
  	int3 pos;
- 	Obj id;
- 	si32 subId;
- 	PlayerColor owner;
- 	ObjectPosInfo();
- 	ObjectPosInfo(const CGObjectInstance * obj);
+	Obj id = Obj::NO_OBJ;
+	si32 subId = -1;
+	PlayerColor owner = PlayerColor::CANNOT_DETERMINE;
+	ObjectPosInfo() = default;
+	ObjectPosInfo(const CGObjectInstance * obj);
 
 	template <typename Handler> void serialize(Handler & h, const int version)
 	{

lib/spells/effects/Damage.cpp

@@ -124,8 +124,8 @@ int64_t Damage::damageForTarget(size_t targetIndex, const Mechanics * m, const b
 
 	if(chainLength > 1 && targetIndex > 0)
 	{
-		double indexedFactor = std::pow(chainFactor, (double) targetIndex);
-		return (int64_t) (indexedFactor * baseDamage);
+		double indexedFactor = std::pow(chainFactor, static_cast<double>(targetIndex));
+		return static_cast<int64_t>(indexedFactor * baseDamage);
 	}
 
 	return baseDamage;
@@ -165,7 +165,7 @@ void Damage::describeEffect(std::vector<MetaString> & log, const Mechanics * m,
 			std::string text = VLC->generaltexth->allTexts[343]; //Does %d points of damage.
 			boost::algorithm::trim(text);
 			line << text;
-			line.addReplacement((int)damage); //no more text afterwards
+			line.addReplacement(static_cast<int>(damage)); //no more text afterwards
 			log.push_back(line);
 		}
 	}
@@ -175,7 +175,7 @@ void Damage::describeEffect(std::vector<MetaString> & log, const Mechanics * m,
 			MetaString line;
 			line.addTxt(MetaString::GENERAL_TXT, 376); // Spell %s does %d damage
 			line.addReplacement(MetaString::SPELL_NAME, m->getSpellIndex());
-			line.addReplacement((int)damage);
+			line.addReplacement(static_cast<int>(damage));
 
 			log.push_back(line);
 		}

lib/spells/effects/Effects.cpp

@@ -25,10 +25,6 @@ namespace spells
 namespace effects
 {
 
-Effects::Effects() = default;
-
-Effects::~Effects() = default;
-
 void Effects::add(const std::string & name, const std::shared_ptr<Effect>& effect, const int level)
 {
 	effect->name = name;

lib/spells/effects/Effects.h

@@ -30,8 +30,7 @@ public:
 
 	EffectData data;
 
-	Effects();
-	virtual ~Effects();
+	virtual ~Effects() = default;
 
 	void add(const std::string & name, const std::shared_ptr<Effect>& effect, const int level);
 

lib/spells/effects/Obstacle.cpp

@@ -68,7 +68,7 @@ static void serializeRelativeShape(JsonSerializeFormat & handler, const std::str
 
 				if(!handler.saving)
 				{
-					value.at(outerIndex).at(innerIndex) = (BattleHex::EDir) vstd::find_pos(EDirMap, temp);
+					value.at(outerIndex).at(innerIndex) = static_cast<BattleHex::EDir>(vstd::find_pos(EDirMap, temp));
 				}
 			}
 		}
@@ -189,7 +189,7 @@ void Obstacle::apply(ServerCallback * server, const Mechanics * m, const EffectT
 		}
 		RandomGeneratorUtil::randomShuffle(availableTiles, *server->getRNG());
 
-		const int patchesToPut = std::min(patchCount, (int)availableTiles.size());
+		const int patchesToPut = std::min(patchCount, static_cast<int>(availableTiles.size()));
 
 		EffectTarget randomTarget;
 		randomTarget.reserve(patchesToPut);

lib/spells/effects/Registry.h

@@ -29,7 +29,6 @@ namespace effects
 class DLL_LINKAGE IEffectFactory
 {
 public:
-	IEffectFactory() = default;
 	virtual ~IEffectFactory() = default;
 
 	virtual Effect * create() const = 0;
@@ -47,7 +46,6 @@ public:
 
 class DLL_LINKAGE GlobalRegistry
 {
-	GlobalRegistry() = default;
 public:
     static Registry * get();
 };
@@ -56,9 +54,6 @@ template<typename E>
 class EffectFactory : public IEffectFactory
 {
 public:
-	EffectFactory() = default;
-	virtual ~EffectFactory() = default;
-
 	Effect * create() const override
 	{
 		return new E();

scripting/lua/LuaScriptingContext.cpp

@@ -529,7 +529,7 @@ int LuaContext::loadModule()
 
 		auto rawData = loader->load(id)->readAll();
 
-		auto sourceText = std::string((char *)rawData.first.get(), rawData.second);
+		auto sourceText = std::string(reinterpret_cast<char *>(rawData.first.get()), rawData.second);
 
 		int ret = luaL_loadbuffer(L, sourceText.c_str(), sourceText.size(), modulePath.c_str());