Merge #6592

6592: Minor refactoring of map and datafile r=def- a=Robyt3



## Checklist

- [ ] Tested the change ingame
- [ ] Provided screenshots if it is a visual change
- [ ] Tested in combination with possibly related configuration options
- [ ] Written a unit test (especially base/) or added coverage to integration test
- [ ] Considered possible null pointers and out of bounds array indexing
- [ ] Changed no physics that affect existing maps
- [ ] Tested the change with [ASan+UBSan or valgrind's memcheck](https://github.com/ddnet/ddnet/#using-addresssanitizer--undefinedbehavioursanitizer-or-valgrinds-memcheck) (optional)


Co-authored-by: Robert Müller <robytemueller@gmail.com>

src/engine/map.h

@@ -16,14 +16,16 @@ class IMap : public IInterface
 	MACRO_INTERFACE("map", 0)
 public:
 	virtual void *GetData(int Index) = 0;
-	virtual int GetDataSize(int Index) = 0;
+	virtual int GetDataSize(int Index) const = 0;
 	virtual void *GetDataSwapped(int Index) = 0;
 	virtual void UnloadData(int Index) = 0;
+	virtual int NumData() const = 0;
+
 	virtual void *GetItem(int Index, int *pType = nullptr, int *pID = nullptr) = 0;
 	virtual int GetItemSize(int Index) = 0;
 	virtual void GetType(int Type, int *pStart, int *pNum) = 0;
 	virtual void *FindItem(int Type, int ID) = 0;
-	virtual int NumItems() = 0;
+	virtual int NumItems() const = 0;
 };
 
 class IEngineMap : public IMap
@@ -31,12 +33,13 @@ class IEngineMap : public IMap
 	MACRO_INTERFACE("enginemap", 0)
 public:
 	virtual bool Load(const char *pMapName) = 0;
-	virtual bool IsLoaded() = 0;
 	virtual void Unload() = 0;
-	virtual SHA256_DIGEST Sha256() = 0;
+	virtual bool IsLoaded() const = 0;
-	virtual unsigned Crc() = 0;
+	virtual IOHANDLE File() const = 0;
-	virtual int MapSize() = 0;
+
-	virtual IOHANDLE File() = 0;
+	virtual SHA256_DIGEST Sha256() const = 0;
+	virtual unsigned Crc() const = 0;
+	virtual int MapSize() const = 0;
 };
 
 extern IEngineMap *CreateEngineMap();

src/engine/shared/datafile.cpp

@@ -217,6 +217,28 @@ bool CDataFileReader::Open(class IStorage *pStorage, const char *pFilename, int
 	return true;
 }
 
+bool CDataFileReader::Close()
+{
+	if(!m_pDataFile)
+		return true;
+
+	// free the data that is loaded
+	for(int i = 0; i < m_pDataFile->m_Header.m_NumRawData; i++)
+		free(m_pDataFile->m_ppDataPtrs[i]);
+
+	io_close(m_pDataFile->m_File);
+	free(m_pDataFile);
+	m_pDataFile = nullptr;
+	return true;
+}
+
+IOHANDLE CDataFileReader::File() const
+{
+	if(!m_pDataFile)
+		return 0;
+	return m_pDataFile->m_File;
+}
+
 int CDataFileReader::NumData() const
 {
 	if(!m_pDataFile)
@@ -227,7 +249,7 @@ int CDataFileReader::NumData() const
 }
 
 // returns the size in the file
-int CDataFileReader::GetFileDataSize(int Index)
+int CDataFileReader::GetFileDataSize(int Index) const
 {
 	if(!m_pDataFile)
 	{
@@ -240,7 +262,7 @@ int CDataFileReader::GetFileDataSize(int Index)
 }
 
 // returns the size of the resulting data
-int CDataFileReader::GetDataSize(int Index)
+int CDataFileReader::GetDataSize(int Index) const
 {
 	if(!m_pDataFile)
 	{
@@ -257,11 +279,11 @@ void *CDataFileReader::GetDataImpl(int Index, int Swap)
 {
 	if(!m_pDataFile)
 	{
-		return 0;
+		return nullptr;
 	}
 
 	if(Index < 0 || Index >= m_pDataFile->m_Header.m_NumRawData)
-		return 0;
+		return nullptr;
 
 	// load it if needed
 	if(!m_pDataFile->m_ppDataPtrs[Index])
@@ -331,7 +353,7 @@ void CDataFileReader::UnloadData(int Index)
 
 	//
 	free(m_pDataFile->m_ppDataPtrs[Index]);
-	m_pDataFile->m_ppDataPtrs[Index] = 0x0;
+	m_pDataFile->m_ppDataPtrs[Index] = nullptr;
 }
 
 int CDataFileReader::GetItemSize(int Index) const
@@ -391,7 +413,7 @@ void *CDataFileReader::GetItem(int Index, int *pType, int *pID)
 			*pType = 0;
 		if(pID)
 			*pID = 0;
-		return 0;
+		return nullptr;
 	}
 
 	CDatafileItem *pItem = (CDatafileItem *)(m_pDataFile->m_Info.m_pItemStart + m_pDataFile->m_Info.m_pItemOffsets[Index]);
@@ -453,7 +475,7 @@ void *CDataFileReader::FindItem(int Type, int ID)
 	int Index = FindItemIndex(Type, ID);
 	if(Index < 0)
 	{
-		return 0;
+		return nullptr;
 	}
 	return GetItem(Index);
 }
@@ -465,22 +487,6 @@ int CDataFileReader::NumItems() const
 	return m_pDataFile->m_Header.m_NumItems;
 }
 
-bool CDataFileReader::Close()
-{
-	if(!m_pDataFile)
-		return true;
-
-	// free the data that is loaded
-	int i;
-	for(i = 0; i < m_pDataFile->m_Header.m_NumRawData; i++)
-		free(m_pDataFile->m_ppDataPtrs[i]);
-
-	io_close(m_pDataFile->m_File);
-	free(m_pDataFile);
-	m_pDataFile = 0;
-	return true;
-}
-
 SHA256_DIGEST CDataFileReader::Sha256() const
 {
 	if(!m_pDataFile)
@@ -509,13 +515,6 @@ int CDataFileReader::MapSize() const
 	return m_pDataFile->m_Header.m_Size + 16;
 }
 
-IOHANDLE CDataFileReader::File()
-{
-	if(!m_pDataFile)
-		return 0;
-	return m_pDataFile->m_File;
-}
-
 CDataFileWriter::CDataFileWriter()
 {
 	m_File = 0;
@@ -527,15 +526,15 @@ CDataFileWriter::CDataFileWriter()
 CDataFileWriter::~CDataFileWriter()
 {
 	free(m_pItemTypes);
-	m_pItemTypes = 0;
+	m_pItemTypes = nullptr;
 	for(int i = 0; i < m_NumItems; i++)
 		free(m_pItems[i].m_pData);
 	for(int i = 0; i < m_NumDatas; ++i)
 		free(m_pDatas[i].m_pCompressedData);
 	free(m_pItems);
-	m_pItems = 0;
+	m_pItems = nullptr;
 	free(m_pDatas);
-	m_pDatas = 0;
+	m_pDatas = nullptr;
 }
 
 bool CDataFileWriter::OpenFile(class IStorage *pStorage, const char *pFilename, int StorageType)
@@ -568,7 +567,7 @@ bool CDataFileWriter::Open(class IStorage *pStorage, const char *pFilename, int
 	return OpenFile(pStorage, pFilename, StorageType);
 }
 
-int CDataFileWriter::GetTypeFromIndex(int Index)
+int CDataFileWriter::GetTypeFromIndex(int Index) const
 {
 	return ITEMTYPE_EX - Index - 1;
 }
@@ -678,16 +677,12 @@ int CDataFileWriter::Finish()
 	if(!m_File)
 		return 1;
 
-	int ItemSize = 0;
-	int TypesSize, HeaderSize, OffsetSize, FileSize, SwapSize;
-	int DataSize = 0;
-	CDatafileHeader Header;
-
 	// we should now write this file!
 	if(DEBUG)
 		dbg_msg("datafile", "writing");
 
 	// calculate sizes
+	int ItemSize = 0;
 	for(int i = 0; i < m_NumItems; i++)
 	{
 		if(DEBUG)
@@ -695,23 +690,23 @@ int CDataFileWriter::Finish()
 		ItemSize += m_pItems[i].m_Size + sizeof(CDatafileItem);
 	}
 
+	int DataSize = 0;
 	for(int i = 0; i < m_NumDatas; i++)
 		DataSize += m_pDatas[i].m_CompressedSize;
 
 	// calculate the complete size
-	TypesSize = m_NumItemTypes * sizeof(CDatafileItemType);
+	const int TypesSize = m_NumItemTypes * sizeof(CDatafileItemType);
-	HeaderSize = sizeof(CDatafileHeader);
+	const int HeaderSize = sizeof(CDatafileHeader);
-	OffsetSize = (m_NumItems + m_NumDatas + m_NumDatas) * sizeof(int); // ItemOffsets, DataOffsets, DataUncompressedSizes
+	const int OffsetSize = (m_NumItems + m_NumDatas + m_NumDatas) * sizeof(int); // ItemOffsets, DataOffsets, DataUncompressedSizes
-	FileSize = HeaderSize + TypesSize + OffsetSize + ItemSize + DataSize;
+	const int FileSize = HeaderSize + TypesSize + OffsetSize + ItemSize + DataSize;
-	SwapSize = FileSize - DataSize;
+	const int SwapSize = FileSize - DataSize;
-
-	(void)SwapSize;
 
 	if(DEBUG)
 		dbg_msg("datafile", "num_m_aItemTypes=%d TypesSize=%d m_aItemsize=%d DataSize=%d", m_NumItemTypes, TypesSize, ItemSize, DataSize);
 
 	// construct Header
 	{
+		CDatafileHeader Header;
 		Header.m_aID[0] = 'D';
 		Header.m_aID[1] = 'A';
 		Header.m_aID[2] = 'T';
@@ -843,12 +838,12 @@ int CDataFileWriter::Finish()
 	for(int i = 0; i < m_NumItems; i++)
 	{
 		free(m_pItems[i].m_pData);
-		m_pItems[i].m_pData = 0;
+		m_pItems[i].m_pData = nullptr;
 	}
 	for(int i = 0; i < m_NumDatas; ++i)
 	{
 		free(m_pDatas[i].m_pCompressedData);
-		m_pDatas[i].m_pCompressedData = 0;
+		m_pDatas[i].m_pCompressedData = nullptr;
 	}
 
 	io_close(m_File);

src/engine/shared/datafile.h

@@ -20,7 +20,7 @@ class CDataFileReader
 {
 	struct CDatafile *m_pDataFile;
 	void *GetDataImpl(int Index, int Swap);
-	int GetFileDataSize(int Index);
+	int GetFileDataSize(int Index) const;
 
 	int GetExternalItemType(int InternalType);
 	int GetInternalItemType(int ExternalType);
@@ -30,28 +30,27 @@ public:
 		m_pDataFile(nullptr) {}
 	~CDataFileReader() { Close(); }
 
-	bool IsOpen() const { return m_pDataFile != nullptr; }
-
 	bool Open(class IStorage *pStorage, const char *pFilename, int StorageType);
 	bool Close();
+	bool IsOpen() const { return m_pDataFile != nullptr; }
+	IOHANDLE File() const;
 
 	void *GetData(int Index);
 	void *GetDataSwapped(int Index); // makes sure that the data is 32bit LE ints when saved
-	int GetDataSize(int Index);
+	int GetDataSize(int Index) const;
 	void UnloadData(int Index);
+	int NumData() const;
+
 	void *GetItem(int Index, int *pType = nullptr, int *pID = nullptr);
 	int GetItemSize(int Index) const;
 	void GetType(int Type, int *pStart, int *pNum);
 	int FindItemIndex(int Type, int ID);
 	void *FindItem(int Type, int ID);
 	int NumItems() const;
-	int NumData() const;
-	void Unload();
 
 	SHA256_DIGEST Sha256() const;
 	unsigned Crc() const;
 	int MapSize() const;
-	IOHANDLE File();
 };
 
 // write access
@@ -99,8 +98,8 @@ class CDataFileWriter
 	CDataInfo *m_pDatas;
 	int m_aExtendedItemTypes[MAX_EXTENDED_ITEM_TYPES];
 
+	int GetTypeFromIndex(int Index) const;
 	int GetExtendedItemTypeIndex(int Type);
-	int GetTypeFromIndex(int Index);
 
 public:
 	CDataFileWriter();

src/engine/shared/map.cpp

@@ -9,44 +9,52 @@ void *CMap::GetData(int Index)
 {
 	return m_DataFile.GetData(Index);
 }
-int CMap::GetDataSize(int Index)
+
+int CMap::GetDataSize(int Index) const
 {
 	return m_DataFile.GetDataSize(Index);
 }
+
 void *CMap::GetDataSwapped(int Index)
 {
 	return m_DataFile.GetDataSwapped(Index);
 }
+
 void CMap::UnloadData(int Index)
 {
 	m_DataFile.UnloadData(Index);
 }
+
+int CMap::NumData() const
+{
+	return m_DataFile.NumData();
+}
+
 void *CMap::GetItem(int Index, int *pType, int *pID)
 {
 	return m_DataFile.GetItem(Index, pType, pID);
 }
+
 int CMap::GetItemSize(int Index)
 {
 	return m_DataFile.GetItemSize(Index);
 }
+
 void CMap::GetType(int Type, int *pStart, int *pNum)
 {
 	m_DataFile.GetType(Type, pStart, pNum);
 }
+
 void *CMap::FindItem(int Type, int ID)
 {
 	return m_DataFile.FindItem(Type, ID);
 }
-int CMap::NumItems()
+
+int CMap::NumItems() const
 {
 	return m_DataFile.NumItems();
 }
 
-void CMap::Unload()
-{
-	m_DataFile.Close();
-}
-
 bool CMap::Load(const char *pMapName)
 {
 	IStorage *pStorage = Kernel()->RequestInterface<IStorage>();
@@ -55,29 +63,34 @@ bool CMap::Load(const char *pMapName)
 	return m_DataFile.Open(pStorage, pMapName, IStorage::TYPE_ALL);
 }
 
-bool CMap::IsLoaded()
+void CMap::Unload()
+{
+	m_DataFile.Close();
+}
+
+bool CMap::IsLoaded() const
 {
 	return m_DataFile.IsOpen();
 }
 
-SHA256_DIGEST CMap::Sha256()
+IOHANDLE CMap::File() const
-{
-	return m_DataFile.Sha256();
-}
-
-unsigned CMap::Crc()
-{
-	return m_DataFile.Crc();
-}
-
-int CMap::MapSize()
-{
-	return m_DataFile.MapSize();
-}
-
-IOHANDLE CMap::File()
 {
 	return m_DataFile.File();
 }
 
+SHA256_DIGEST CMap::Sha256() const
+{
+	return m_DataFile.Sha256();
+}
+
+unsigned CMap::Crc() const
+{
+	return m_DataFile.Crc();
+}
+
+int CMap::MapSize() const
+{
+	return m_DataFile.MapSize();
+}
+
 extern IEngineMap *CreateEngineMap() { return new CMap; }

src/engine/shared/map.h

@@ -16,28 +16,25 @@ public:
 	CMap();
 
 	void *GetData(int Index) override;
-	int GetDataSize(int Index) override;
+	int GetDataSize(int Index) const override;
 	void *GetDataSwapped(int Index) override;
 	void UnloadData(int Index) override;
-	void *GetItem(int Index, int *pType, int *pID) override;
+	int NumData() const override;
+
+	void *GetItem(int Index, int *pType = nullptr, int *pID = nullptr) override;
 	int GetItemSize(int Index) override;
 	void GetType(int Type, int *pStart, int *pNum) override;
 	void *FindItem(int Type, int ID) override;
-	int NumItems() override;
+	int NumItems() const override;
-
-	void Unload() override;
 
 	bool Load(const char *pMapName) override;
+	void Unload() override;
+	bool IsLoaded() const override;
+	IOHANDLE File() const override;
 
-	bool IsLoaded() override;
+	SHA256_DIGEST Sha256() const override;
-
+	unsigned Crc() const override;
-	SHA256_DIGEST Sha256() override;
+	int MapSize() const override;
-
-	unsigned Crc() override;
-
-	int MapSize() override;
-
-	IOHANDLE File() override;
 };
 
 #endif