-
Notifications
You must be signed in to change notification settings - Fork 655
Add printf formatting macros for nk data types #773
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
base: master
Are you sure you want to change the base?
Conversation
|
Here's a test program to check for warnings. https://gist.github.com/PROP65/88cf2ec23a5052350f2e79857dd9fd8e |
|
Seems like a good solution. Those warnings have been pretty annoying in the build sometimes. |
RobLoach
left a comment
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.
Seems good to me. Going to leave this open for a bit for others to review too.
klei1984
left a comment
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 am using msys2/Mingw64 toolchain on Windows which has some problems with the NK_INCLUDE_FIXED_TYPES feature. See in-line review comments.
| /* progressbar combobox */ | ||
| sum = prog_a + prog_b + prog_c + prog_d; | ||
| sprintf(buffer, "%lu", sum); | ||
| sprintf(buffer, "%" NK_FMT_SIZE, sum); |
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.
On Windows 64 bit, msys2/mingw64 this line gives the following warning message if demo is built in -std=c89 mode.
Compiler settings:
CFLAGS += -std=c89 -Wall -Wextra -pedantic -ggdb -O0 -DSDL_DISABLE_IMMINTRIN_H
CFLAGS += `sdl2-config --cflags`
Warning message:
In file included from main.c:63:
../../demo/common/overview.c: In function 'overview':
../../demo/common/overview.c:447:33: warning: ISO C does not support the 'I64' ms_printf length modi
fier [-Wformat=]
447 | sprintf(buffer, "%" NK_FMT_SIZE, sum);
| ^~~
NK_INCLUDE_FIXED_TYPES is active in the above context.
Disabling the macro in the 64 bit build environment fails via assertions like:
NK_STATIC_ASSERT(sizeof(nk_size) >= sizeof(void*));
NK_STATIC_ASSERT(sizeof(nk_ptr) >= sizeof(void*));
The potential reason is this:
#elif defined(__GNUC__) || defined(__clang__)
#if defined(__x86_64__) || defined(__ppc64__) || defined(__PPC64__) || defined(__aarch64__)
#define NK_SIZE_TYPE unsigned long
#else
On Windows 64 bit, msys2/mingw64 unsigned long is 4 bytes.
Architecture of built software:
architecture: i386:x86-64, flags 0x0000013b:
HAS_RELOC, EXEC_P, HAS_DEBUG, HAS_SYMS, HAS_LOCALS, D_PAGED
start address 0x00000001400013f0
The 32 bit build environment, using msys2/mingw32, builds without errors and warnings.
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 added checks for MinGW; the static asserts should be fixed. Since there isn't any type in c89 that is guaranteed to be at least 64 bits, it may not be possible on 64 bit windows targets to format nk_size in a way that is c89 compliant.
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 checked both NK_INCLUDE_FIXED_TYPES active and inactive in 32/64 bit and c89/c99/c11/c17/c23 combinations.
- I can confirm that the demo I checked earlier is now not tripping on the assertion tests in any checked compilation modes.
The rest are warnings and as noted by PRP65, most are C ISO standards related.
- c89 raises
long longwarnings in 64 bit mode which is perfectly fine. - in case fixed types macro is disabled, 32 bit mode in c89 mode warns about no support for I32 printf flag.
- in case fixed types macro is disabled, 32/64 bit modes in c99/C11/C17/C23 modes warn about no support for I printf flag.
- in case fixed types macro is disabled, 64 bit mode in c99/C11/C17/C23 modes warn about NK_FMT_SIZE being incompatible with size_t. Warning example below.
../../demo/common/overview.c:447:33: warning: format '%u' expects argument of type 'unsigned int', but argument 3 has type 'size_t' {aka 'long long unsigned int'} [-Wformat=]
447 | sprintf(buffer, "%" NK_FMT_SIZE, sum);
| ^~~ ~~~
| |
| size_t {aka long long unsigned int}
| /* checkbox combobox */ | ||
| sum = (size_t)(check_values[0] + check_values[1] + check_values[2] + check_values[3] + check_values[4]); | ||
| sprintf(buffer, "%lu", sum); | ||
| sprintf(buffer, "%" NK_FMT_SIZE, sum); |
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.
Warning message:
../../demo/common/overview.c:459:33: warning: ISO C does not support the 'I64' ms_printf length modi
fier [-Wformat=]
459 | sprintf(buffer, "%" NK_FMT_SIZE, sum);
|
|
IS this good to go, now? |
|
Is this really necessary? Nuklear does not depend on exact-int-size printf formatting anywhere in its sources, only demos and examples really need it. There is roughly one place in a library where a simple formatting ( @RobLoach Don't get me wrong, but I don't think this should be ever merged. The linked issue (#727) is only about |
Adds macros from formatting the nk data types
This won't completely prevent issues like #727 but it should help.
Needs to be tested on Windows.