Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enhancement] Use correct data types #605

Open
smallmodel opened this issue Jul 3, 2023 · 2 comments
Open

[Enhancement] Use correct data types #605

smallmodel opened this issue Jul 3, 2023 · 2 comments

Comments

@smallmodel
Copy link

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

  • Use a const char* instead of a char* when the string doesn't need to be edited
  • Use a size_t instead of an int for memory allocations or for any pointer-related stuff like a pointer size (useful for x64)
  • Use a 64-bit integer for file-related functions

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.

@smallmodel smallmodel changed the title Use correct data types [Enhancement] Use correct data types Jul 3, 2023
@timangus
Copy link
Member

timangus commented Jul 3, 2023

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

@smallmodel
Copy link
Author

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 :

  • Different struct size, different field offsets (i.e in trace_t, gentity_t, ...)
  • Renderer module would have issues with imported/exported functions

(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. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants