Skip to content

Comments

make qoi.h build using c89 compilers.#67

Merged
phoboslab merged 1 commit intophoboslab:masterfrom
sezero:c89
Dec 13, 2021
Merged

make qoi.h build using c89 compilers.#67
phoboslab merged 1 commit intophoboslab:masterfrom
sezero:c89

Conversation

@sezero
Copy link
Contributor

@sezero sezero commented Dec 5, 2021

This patch makes qoi.h to build using c89 compilers.

Note: Even with this, my gcc emits the following warnings:

qoi.h: In function 'qoi_encode':
qoi.h:356: warning: missing braces around initializer
qoi.h:356: warning: (near initialization for 'index[0]')
qoi.h: In function 'qoi_decode':
qoi.h:497: warning: missing braces around initializer
qoi.h:497: warning: (near initialization for 'index[0]')

These are qoi_rgba_t index[64] = {0}; and a compiler (e.g. MSVC)
can optimize those to memset(index,0,sizeof(index)) which can be
a problem with users of the header. I suggest that qoi project
expose a QOI_ZERO macro and use it instead of those initializers.
E.g.:

#ifndef QOI_ZEROARR
# define QOI_ZEROARR(_arr) memset((_arr),0,sizeof(_arr))
#endif

@sezero
Copy link
Contributor Author

sezero commented Dec 6, 2021

I went ahead and implemented my suggestion of QOI_ZEROARR

@sezero
Copy link
Contributor Author

sezero commented Dec 11, 2021

Rebased to current master.

@sezero
Copy link
Contributor Author

sezero commented Dec 11, 2021

Rebased (again) + revised.

also add a QOI_ZEROARR macro, wrapping around memset by default.
@sezero
Copy link
Contributor Author

sezero commented Dec 13, 2021

Rebased

@phoboslab
Copy link
Owner

I'm not a fan of having all variable declarations at the top of the functions, but of course not opposed to that if it's required.

I'm confused about the actual standards though. I compiled with gcc qoiconv.c -std=c89 -o qoiconv (gcc version 8.3.0). My compiler complained about:

qoi.h:399:2: error: ‘for’ loop initial declarations are only allowed in C99 or C11 mode
  for (int px_pos = 0; px_pos < px_len; px_pos += channels) {
  ^~~

Fair enough.

qoi.h:226:1: error: C++ style comments are not allowed in ISO C90
 // -----------------------------------------------------------------------------
 ^

So we can't use single line comments if we want to conform to c89? Meh...

Only(!) when I compile with -pedantic I get

qoi.h:422:4: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
    int index_pos = QOI_COLOR_HASH(px) % 64;
    ^~~

and

qoi.h:517:19: warning: ISO C90 forbids specifying subobject to initialize [-Wpedantic]
  qoi_rgba_t px = {.rgba = {.r = 0, .g = 0, .b = 0, .a = 0}};
                   ^

The one thing my gcc did not(!) complain about was the {0} initializer and according to SO it seems to be valid c89 after all. So maybe your (or my??) gcc just needs an update?

So, which standard and/or compiler exactly are we targeting here? Is this just "whatever SDL is compiled with", or is there some more universal guidelines of what exactly is required?

@sezero
Copy link
Contributor Author

sezero commented Dec 13, 2021

I'm not a fan of having all variable declarations at the top of the functions, but of course not opposed to that if it's required.

Those changes are to avoid -Werror==declaration-after-statement
Any some older MSVC versions (2013 and older? not exactl sure) versions error
out immediately with them

qoi.h:399:2: error: ‘for’ loop initial declarations are only allowed in C99 or C11 mode

... and those

qoi.h:517:19: warning: ISO C90 forbids specifying subobject to initialize [-Wpedantic]
qoi_rgba_t px = {.rgba = {.r = 0, .g = 0, .b = 0, .a = 0}};

C99 initializers aren't supported by older non-gcc (older MSVC for e.g.,
not exactly sure about which versions.)

The one thing my gcc did not(!) complain about was the {0} initializer

That's not about c99 or c89 or c11: see the original note: if the compiler
emits a memset for those and the qoi.h user isn't linking to libc, there is
your problem.

So, which standard and/or compiler exactly are we targeting here? Is this just "whatever SDL is compiled with", or is there some more universal guidelines of what exactly is required?

These are suggestions only. You decide what standards you want to
stick to, and we can modify things if it doesn't meet ours.

@phoboslab
Copy link
Owner

That's not about c99 or c89 or c11: see the original note: if the compiler emits a memset for those and the qoi.h user isn't linking to libc, there is your problem.

Makes sense. Thanks for clarifying!

Interestingly SDL_image at some point apparently honored the "no // comments" c89 standard (e.g. IMG_bmp.c - not a single //) but later abandoned it (e.g. IMG_xcf.c).

C99 initializers aren't supported by older non-gcc (older MSVC for e.g., not exactly sure about which versions.)

SDL itself seems to allow designated initializers, but only in code that targets linux, QNX, PSP or in test/ (so no MSVC!?).

Also, thanks for your work and sorry if this came off blunt. I'm very excited to see QOI in SDL! I just wanted to know the motivation for these specific changes.

I'll merge this soon. But I guess it would be a good idea for QOI to go full-c89, instead of MSVC's "c89 with benefits", if we're halfway there anyway...

@sezero
Copy link
Contributor Author

sezero commented Dec 13, 2021

Interestingly SDL_image at some point apparently honored the "no // comments" c89 standard (e.g. IMG_bmp.c - not a single //) but later abandoned it (e.g. IMG_xcf.c).

AFAIK, SDL usually lives OK with single line comments within its own code
but tries to avoid them in its public headers for better compatibility.

C99 initializers aren't supported by older non-gcc (older MSVC for e.g., not exactly sure about which versions.)

SDL itself seems to allow designated initializers, but only in code that targets linux, QNX, PSP or in test/ (so no MSVC!?).

Yes, the gcc-only parts are more lenient. (I hadn't noticed designated initializers
under test/ though, will look.)

I'll merge this soon. But I guess it would be a good idea for QOI to go full-c89, instead of MSVC's "c89 with benefits", if we're halfway there anyway...

Thanks.

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

Successfully merging this pull request may close these issues.

2 participants