ioquake / ioq3

The ioquake3 community effort to continue supporting/developing id's Quake III Arena
https://ioquake3.org/
GNU General Public License v2.0
2.42k stars 529 forks source link

[Enhancement] Use correct data types #605

Open smallmodel opened 1 year ago

smallmodel commented 1 year ago

The suggestion is to fix all data-type related mistakes, like for example :

const fix example:

//void Sys_ListFilteredFiles( const char *basedir, char *subdirs, char *filter, char **list, int *numfiles );
// Should be:
void Sys_ListFilteredFiles( const char *basedir, const char *subdirs, const char *filter, char **list, unsigned int *numfiles );
// Maybe use an uint32_t for number of files?

Using const means the caller knows that the string will never be modified, it would be then more safe to use string literals as input.

size_t example:

//void *Z_Malloc(int size);
void *Z_Malloc(size_t size);
void function(const char *str) {
  //int len = strlen(str);
  // strlen() returns a size_t, so use a size_t
  size_t len = strlen(str);
  for(size_t i = 0; i < len; i++) {
    // ...
  }
}

By using size_t, the caller knows that the function's input is a size-type. It would be more consistent with x64 code, it could be possible to allocate memory above 4GB (although it won't ever happen), and it would be more compatible with the C standard library that return size_t. It would also mean that there will never be a size below 0 as size_t is unsigned.

FS size example:

//int       FS_FTell( fileHandle_t f );
//int       FS_Seek( fileHandle_t f, long offset, fsOrigin_t origin );
int64_t     FS_FTell( fileHandle_t f );
int         FS_Seek( fileHandle_t f, int64_t offset, fsOrigin_t origin );

// By the way currently, FS_FTell() returns int, and FS_Seek accepts a long as offset, datatypes don't match

Opening files above 4GB could then be possible.

I'm aware about the challenge of fixing the whole codebase. 🙂 Having the correct type can hint developers about the usage.

timangus commented 1 year ago

A laudable goal, but there are many, many spots where the types can't change as doing so would break API/ABI compatibility.

smallmodel commented 1 year ago

Oh yes I understand, that's the disadvantage. I see where it would break, if ioq3 gets compiled with new structures it would break existing binaries because :

(Atm I don't see anything else)

Modules must have to be recompiled.

Modifying functions shouldn't cause issues for game/cgame modules as they use traps (if they remain unchanged). And the renderer module could get "proxy" functions as import (like CL_RefMalloc) with the exact parameters it wants, the issue would be the refexport_t structure. Also, using const char*/unsigned (where appropriate), mustn't cause issues either. This one may be the first starting point for a cleaner code base that wouldn't break ABI compatibility.

About structs, maybe create new integer types? For example, qsize_t for size_t: it will be set to the correct type, but would remain 32-bit with a preprocessor definition that tells "Keep ABI compatibility". Then qint64_t, quint64_t (there is already qint64 for qvm compatibility), or maybe qlong_t, qulong_t: always 64-bit, but would be 32-bit when keeping ABI compatibility. That's my 2 cents. 😉