[hatari-devel] Using more GCC warnings?

[Nicolas Pomarède]

Le 18/09/2010 10:05, Eero Tamminen a écrit :
> Hi,
>
> On Saturday 18 September 2010, Thomas Huth wrote:
>>> I already fixed couple of things this complains about, but quite a few
>>> functions would need after -Wextra to be explicitly declared as not
>>> using all of the arguments given to them.
>>
>> I personally dislike -Wextra because it generates a lot of annoying
>> warnings, especially these "unused parameter" warnings. Not using one
>> of the parameters is perfectly legal C, and often also necessary, e.g.
>> to meet certain function interface requirements, so I really don't see
>> the point in polluting the code with those "__unused" keywords. If you
>> really want additional warnings, I would prefer if you use the
>> corresponding -Wsomething option directly, and omit -Wunused-parameter
>> from the list.
>
> There are quite a lot of warning options included into -Wextra:
>             -Wclobbered -Wempty-body -Wignored-qualifiers
>             -Wmissing-field-initializers -Wmissing-parameter-type (C only)
>             -Wold-style-declaration (C only) -Woverride-init -Wsign-compare
>             -Wtype-limits -Wuninitialized (only with -O1 and above)
>             -Wunused-parameter (only with -Wunused or -Wall)
>
> But it also does extra things in addition to those:
>             The option -Wextra also prints warning messages for the following
>             cases:
>             *   A pointer is compared against integer zero with
>                  <,<=,>, or>=.
>             * Lots of C++ specific checks...
>
> So I think "-Wextra -Wno-unused-parameter" is better.

Hello,

I would have the same opinion as Thomas on this, I really don't like 
when code gets crippled by some gcc's specific statements. I think it 
makes the whole code less easy to read and can potentialy stop new 
developers interested in emulation but not in the inner gcc's work.

If someone wants to change its local gcc flags to get some warnings and 
see if they're valid to report them on the mailing list, that's ok for 
me ; but I'm personaly against permanently adding statements that helps 
gcc (and not all target architectures for Hatari have gcc, so maybe 
we're already missing some warnings specific to some other compilers).


> With that the only warning comes from here:
> --------------
> In file included from ./hatari/src/xbios.c:19:
> ./hatari/src/includes/m68000.h: In function ‘M68000_AddCyclesWithPairing’:
> ./hatari/src/includes/m68000.h:263: warning: suggest braces around empty
> body in an ‘else’ statement
> ---------------
>
> Maybe that could be fixed?

I could add a {;} statement in the else, but as it doesn't produce 
warnings with the current flags, I'm not sure this is needed unless we 
change the flags.


>
> Btw. there are also these:
> $ grep CMAKE_C_FLAGS $(find -name CMakeLists.txt)
> gui-sdl/CMakeLists.txt:set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-write-strings")
> uae-cpu/CMakeLists.txt:set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-unused -Wno-sign-compare -Wno-shadow")
>
> Not checking for sign comparison can leave pretty hard to find bugs.

Considering this is in uae cpu's emulation, I'm pretty sure this is for 
speed reason and to avoid unnecessary casts and that's there no bug 
caused by this. Trying to remove no-sign-compare and to fix all the code 
would certainly cause regression and require quite some time to fix.

>
> And at least on Unix, const strings go to a read-only memory section so
> trying to write into them would cause a segfault, so it would be nice to fix
> sdl-gui at some point too (currently it gives a huge amount of warnings
> for that) to find out the issues at compile time instead of run-time...
>

Nicolas