[core] Functions returning arrays inconsistently use unsigned int/int for sizes

[Not-Nik]

Currently, LoadFiledData uses an unsigned int to store the size of what it returns:

RLAPI unsigned char *LoadFileData(const char *fileName, unsigned int *bytesRead);

This is the sensible option in my opinion.

However, CompressData, DecompressData, EncodeDataBase64, DecodeDataBase64 and CodepointToUTF8 all use a normal signed int for their array sizes:

RLAPI unsigned char *CompressData(const unsigned char *data, int dataSize, int *compDataSize);
RLAPI unsigned char *DecompressData(const unsigned char *compData, int compDataSize, int *dataSize);
RLAPI char *EncodeDataBase64(const unsigned char *data, int dataSize, int *outputSize);
RLAPI unsigned char *DecodeDataBase64(const unsigned char *data, int *outputSize);
RLAPI const char *CodepointToUTF8(int codepoint, int *utf8Size);

Let's make this more consistent!

I can create a PR, but you'll need to tell me which of these you prefer. raylib uses signed integers for sizes all over the place so maybe it's time to make some bigger changes in that area; they wouldn't really break existing code and make the library more robust.

[raysan5]

This change could also imply a list of additional changes for consistency and a bunch of type-missmatch warnings...

Here some possible sensible functions that could also require reviewing (but there could be even more):

int GetFileLength(const char *fileName);
int GetPixelDataSize(int width, int height, int format);

Image LoadImageFromMemory(const char *fileType, const unsigned char *fileData, int dataSize);
unsigned char *ExportImageToMemory(Image image, const char *fileType, int *fileSize);

Font LoadFontFromMemory(const char *fileType, const unsigned char *fileData, int dataSize, int fontSize...
GlyphInfo *LoadFontData(const unsigned char *fileData, int dataSize, int fontSize, int *fontChars...

void UpdateMeshBuffer(Mesh mesh, int index, const void *data, int dataSize, int offset);

Wave LoadWaveFromMemory(const char *fileType, const unsigned char *fileData, int dataSize);
Music LoadMusicStreamFromMemory(const char *fileType, const unsigned char *data, int dataSize);
void SetAudioStreamBufferSizeDefault(int size);

Surprisingly, most compression/decompression libraries I checked used just plain int for sizes, I don't know why and I'd like to know it, maybe there is some reason to avoid using unsigned int?

What are the problems related to using just int?

[Not-Nik]

I also found, but forgot to list

RLAPI int *LoadCodepoints(const char *text, int *count);
RLAPI const char **TextSplit(const char *text, char delimiter, int *count);
RLAPI Material *LoadMaterials(const char *fileName, int *materialCount);
// This actually does use unsigned ints
RLAPI ModelAnimation *LoadModelAnimations(const char *fileName, unsigned int *animCount);

I think plain int can be set to -1 to indicate that there was an unrecoverable error, unlike with unsigned int where the best chance to indicate failure is to set the size to 0. This makes sense for general purpose applications where it's vital to properly indicate failure so that specific actions can be taken to mitigate the error. In game development, however, and especially in such a beginner/learning focused library as raylib, I feel like we can get away with just using some hardcoded default in case we can't load assets. This is already the case for fonts and textures/materials where there is a more or less sensible default that doesn't cause applications to crash or behave unreasonably unexpected.

Surprisingly, most compression/decompression libraries I checked used just plain int for sizes, I don't know why and I'd like to know it, maybe there is some reason to avoid using unsigned int?

zlib, the arguably most popular compression library, actually uses unsigned longs for sizes, but returns errors instead of returning values.

I don't think there are inherent problems with int, just that in an attempt at generalisation library developers opted to include error codes in these sizes.

[ghost]

Surprisingly, most compression/decompression libraries I checked used just plain int for sizes, I don't know why and I'd like to know it, maybe there is some reason to avoid using unsigned int?

Perhaps it could be due to the possibility of overflow/underflow. Two good articles I have bookmarked here: "Pitfalls in C and C++: Unsigned types" and "C++ unsigned int problems".

[raysan5]

@ubkp thanks for the articles, very interesting reads. I think they can be used for some specific cases (raw file/memory data?) but not for everything, I also consider that aesthetically, int looks way simpler to a newby than an unsigned int.

I need to think about it, there are too many functions involved in that change and many warnings to review.

[Not-Nik]

These articles make some good points. Either way I think it's important to make a choice and enforce it consistently.

[orcmid]

@ubkp @Not-Nik I find the article to be ridiculous.

The TLDI: I am not proposing any reform to the APIs. Maybe better documentation, something that is not so traumatic.

OBSERVATIONS

First, overflow and underflow problems are the same, whether an int is signed or unsigned, they just occur differently. They wrap either way, because the same hardware operation is performed regardless in 2s-complement arithmetic.

There is a very big difference in comparisons though and that may or may not cause mysteries.

For example, in unsigned int cases, comparisons are very clear. It is also the case that adding one to the largest uint produces the smallest uint (namely 0). Adding one to the largest non-negative int produces the smallest (very-negative) int. Subtracting one from the smallest int produces ... guess what. It is an useful exercise to work out these cases and also the magical case that the smallest int is its own negative.

The use of int to also allow for exception cases is a serious relic in the C standard library (and its use in C++). This applies to exit() and also main(). It applies to most of the stdio.h operations and is probably better than checking errno elsewhere in the face of thread issues. I suggest that overloading the use of int in this manner is best confined to function results and not values communicated in and out of functions via parameters, at least in APIs of our own design.

I hazard that novices are not exposed to the above situations very much and I don't see much concern for checking int results from Standard Library functions either. Since these edge cases are wired into Standard C/C++ and modern processors, the question of how to protect novices and experts from unintended consequences is similar.

CONCLUSION

I don't think the problem at hand is the inescapable coyness of C/C++ integer representations. It is more about what a particular variable and type are intended to represent. That's why size_t is more descriptive (and portable) where it is used and applicable. Having a 0 value be possible is a pretty graceful fall-down (in contrast with a NULL pointer result).

We need to make clear what an int-typed (signed or unsigned) value is for and what the range expectations are. Ideally, we want a type that is the simplest suited to precisely that purpose.

PS: In the spirit of defensive programming, if an API uses an int parameter for where only non-negative values are acceptable, there is then an obligation to defend/detect/protect against an unwelcome value. This applies to ranges generally of course.

[makimy]

No, the article "Pitfalls in C and C++: Unsigned types" is not rediculus.

First, overflow and underflow problems are the same, whether an int is signed or unsigned

It is not true.

[orcmid]

@makimy

No, the article "Pitfalls in C and C++: Unsigned types" is not rediculus.

First, overflow and underflow problems are the same, whether an int is signed or unsigned

It is not true.

This is silly. I said they have the same problems, just in different ways.

uint's overflow and underflow by wrapping around. In the uint case, the wrap arounds are between 0 and UINTMAX. With int's the wraparounds are between INTMAX and INTMIN. You can wrap in either direction. And you can demonstrate on machines with 2's complement arithmetic, that -INTMIN == INTMIN. Since -0 == 0 (in 2's-complement) for int, there has to be at least one other case of a value that is its own negative.

Just try it. It is easy to demonstrate. Write a small program and satisfy yourself about this. #include <limits.h> to get the relevant minimum and maximum values. (There is no UINTMIN because 0 is easy enough.)

[makimy]

Read the article carefully.

[orcmid]

@makimy

Read the article carefully.

OK, I read it through again. I still think it is bogus. Yes, subtracting off the bottom of an unsigned element or assigning `-1' to an unsigned (which will involve a cast and should be detected unless the warning has been suppressed) is unfortunate.

The key thing is about having to know what are supported ranges and exercising care to avoid any undefended over-/under-flow cases. If our attention is on portability, that will matter the most. C Language gives us the problems, and also the tools to manage it once we get the hang of it.

PS: My concern with recommendations like those in the article is the significant risk that folks will blindly follow the recommendation as if there is then nothing to worry about. I have similar concerns with the casual use of float and hand-waving about how there will certainly be enough precision. Then there's the helplessness that accompanies a breakdown where those assumptions do not hold.

[makimy]

I could start to write a great big comment about importance of performance in games (like nobody knows) , the bigger one about importance of portability (I use "thumb" on ARM by the way). Instead, I will say that I'm glad that you got it, the importance of writing valid, C99 code that raylib introduces as one of the main features, maybe, it does not follow it entirely, but it is ok.

[raysan5]

Finally I decided to review all the functions to be consistent to int. It's probably the simplest type for a newby user, the resolution is not-that-distant to unsigned int (compared with bigger types) and it allows returning -1 in case of errors.

All parameters where also renamed to be consistent with dataSize.