Have AVIF_FALSE and AVIF_TRUE be enums #2712
Conversation
vrabaud
force-pushed
the
ci
branch
3 times, most recently
from
8b06b67 to
a5a18f8
Compare
wantehchang
left a comment
wantehchang
left a comment
There was a problem hiding this comment.
LGTM. Thanks a lot for figuring out a way to let the compiler detect this bug.
Another approach that might work is to compile the .c files as C++ code.
|
I thought about having a CI specific guard, but that's the only thing that would be guarded for now. We could also just use stdbool.h no? (it would not solve this problem though but it would make the code more standard) |
|
Using stdbool actually finds other issues ... #2734 |
vrabaud
force-pushed
the
ci
branch
2 times, most recently
from
6be19e0 to
cface84
Compare
| if: ${{ matrix.case == 2}} | ||
| run: | | ||
| sed -i 's/#define AVIF_TRUE 1/enum avifTrueFalse_{ AVIF_TRUE = 1,/' include/avif/avif.h | ||
| sed -i 's/#define AVIF_FALSE 0/AVIF_FALSE = 0};/' include/avif/avif.h |
There was a problem hiding this comment.
I suggest we add these two alternative definitions of avifBool to avif.h and avoid patching avif.h like this. The alternative definitions would be guarded by macros that we define in this CI workflow.
If we want to patch avif.h, we may want to add comments around the definition of avifBool to prevent people form changing or even reformatting it.
There was a problem hiding this comment.
I think adding two guards (for stdbool and enum) is a lot of noise for a public header no? Especially just for a CI usage (not even a test).
| sed -i 's/typedef int avifBool;/typedef bool avifBool;/' include/avif/avif.h | ||
| sed -i 's/#define AVIF_TRUE 1/#define AVIF_TRUE true/' include/avif/avif.h | ||
| sed -i 's/#define AVIF_FALSE 0/#define AVIF_FALSE false/' include/avif/avif.h | ||
| - name: Use enums for AVIF_TRUE/AVIF_FALSE |
There was a problem hiding this comment.
Do we also need this alternative definition? Does stdbool not catch all the bugs?
There was a problem hiding this comment.
Nope, cf the other PR: both found different bugs .....
3 participants
This offers a solution to see errors like fixed in #2711