-
Notifications
You must be signed in to change notification settings - Fork 329
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
Some refactoring #218
base: master
Are you sure you want to change the base?
Some refactoring #218
Conversation
Adding I'll go through this in a few days. Thanks! |
MSVC ignored stdint.h and inttypes.h (and the rest of C99) for many years, but they once the same features showed up in C++11, they were implemented. From what I can google, they exist in MSVC 2015, and not in 2005; I can't find any more precise bounds. That said, PRIu64 is indeed ugly. And it's not correct either - png_size_t is size_t, which is not a 64bit type. The correct formatting opcode for a size_t is %zu. (I'm also unsure why we'd use png_size_t and not just size_t.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only remarked about the size_t
issue once, but to do it properly this includes a full API update throughout the code.
@@ -657,7 +657,7 @@ void *qoi_read(const char *filename, qoi_desc *desc, int channels) { | |||
return NULL; | |||
} | |||
|
|||
bytes_read = fread(data, 1, size, f); | |||
bytes_read = (int)fread(data, 1, size, f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fread
returns ssize_t
. Casting to int
truncates to 2 GiB. Change size_t size, bytes_read;
above instead.
Also this call to fread
is lacking any error checking.
@@ -131,7 +132,7 @@ void libpng_encode_callback(png_structp png_ptr, png_bytep data, png_size_t leng | |||
write_data->size += length; | |||
} | |||
|
|||
void *libpng_encode(void *pixels, int w, int h, int channels, int *out_len) { | |||
static void *libpng_encode(void *pixels, int w, int h, int channels, int *out_len) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer size_t
for all usages as size/quantities, like w
, h
, channels
and for out_len
.
I will implement a check to ensure |
I had rather thought that
In
I agree your opinion, it needs API updating to fix the issue properly . |
I'm not much of a fan for C89: If code should be as compatible as possible I usually prefer it to follow C99 standard, as several very convenint features like inline functions, an expliit bool data type as well as intermixed variable declarations&code have been introduced (among others). Furthermore the comment style used in lines 38ff of qoi.h is also C99. ;-) Also, strictly speaking Or put differently: Not using the C99 standard introduces more hassles in maintenance than the compatibility argument warrants. I'm not aware of any decent compiler that does not support C99 enough to understand C99 was released 22 years ago: At some point it's just not state of the art anymore … |
@BenBE I want to make two points clear. |
on windows : %I instead of %z something like #ifdef _WIN32
#define FMT_ZU "%Iu"
#else
#define FMT_ZU "%zu"
#endif and use this macro when needed. It will always work, even with old versions of Visual Studio and if there is a warning with gcc about %I being not ISO, add -Wno-pedantic-ms-format to the gcc options |
png_size_t
instead ofint
printf
usinginttypes.h
for portabilityvoid
due to suppress warningsstbi_image_free
instead offree
static
to local symbols-Wall -Wextra
forqoibench
-Wall -Wextra -pedantic-errors
forqoiconv
This PR doesn't change the output of
qoiconv
andqoibench
for the conventionally assumed environments.