Skip to content

Conversation

@countingpine
Copy link
Collaborator

This is my first commit for a while. Doing a PR seems like a nice way to go about this.
This fixes some compiler warnings I came across while making from Ubuntu 18.04 (64-bit).
Most are implicit switch() fall-throughs, two are sprintf() buffer allocation warnings.

Note that I've made one of the fall-throughs into a goto, to match the other cases in that block. The fall-through worked because it was the last case in the block, but could have broken if another case were added. The compiler should be able to optimise it out anyway.

The joystick sprintf() warning was difficult to make a decision about, because the problem is GCC not understanding that j can only be two digits. I extended the buffer, but other approaches might involve changing the type of j, using character manipulation or something to write the digits, or just ignoring the warning.

With these changes, the rtlib and gfxlib compile without warnings in (my version of) Linux.

warning: '%d' directive writing between 1 and 11 bytes into a region of size 9 [-Wformat-overflow=]
note: '__builtin___sprintf_chk' output between 9 and 19 bytes into a destination of size 16
   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1, __bos (__s), __fmt, __va_arg_pack ());

Make filename long enough for '/dev/lpt' + '-1234567890' + '\0'
…h to know the int value can only be two digits.
@jayrm
Copy link
Member

jayrm commented Sep 26, 2018

Nice!

I compiled for win32/64 and see fall through warnings for following lines.

src/rtlib/dev_file_encod_read_core.c: 168, 171, 174, 177, 180
src/rtlib/utf_convfrom_wchar.c: 39, 42, 45, 80, 83, 86
src/rtlib/utf_convto_char.c: 28, 30, 32, 34, 36, 75, 77, 79, 81, 83
src/rtlib/utf_convto_wchar.c: 32, 34, 36, 38, 40, 90, 92, 94, 96, 98
src/rtlib/utf_convto_wchar.c: 148, 150, 152, 154, 156, 192, 194, 196, 198, 200
src/gfxlib2/win32/gfx_win32.c: 329

I didn't try dos (my cross compiler not working).

@countingpine
Copy link
Collaborator Author

Not sure how I missed some of them. Surely GCC would have caught those ones too.. I'll have another look.

The gfxlib code seems quite complex. If it is genuinely supposed to fall through, then the fix is obvious, Might also be worth reformatting the case a bit.

@countingpine
Copy link
Collaborator Author

I've submitted a couple more changes - some warnings appeared the next time I did a clean make, some from your list didn't appear at all, so I looked through and changed them all myself. Hopefully I didn't miss any..

Looking at the win32 gfx code, I guess I can now say that WM_SIZE falls through to WM_SYSKEYDOWN (to handle their Maximize events), and WM_SYSKEYDOWN falls through to WM_KEYDOWN (to handle key events), but WM_SIZE will not fall through to WM_KEYDOWN.

It might be that it can be refactored - the WM_SIZE case could set the value of is_maximize, then break; the WM_SYSKEYDOWN case could set the value of is_alt_enter, then fall through; then is_maximize || is_alt_enter could be checked after the end of the switch block. It would change the order of execution though.

Otherwise the simplest solution is to put a comment in saying something like /* fall through for WM_SYSKEYDOWN /*, which makes the flow clearer, and will probably satisfy GCC. The final if ... break could also be pulled out of the scope.

@countingpine countingpine merged commit df595db into freebasic:master Oct 2, 2018
@countingpine
Copy link
Collaborator Author

Hopefully we've caught all the warnings now. I am committing this. Thanks both for taking the time to look at this. It's been helpful.

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.

3 participants