Re: [hatari-devel] fixing errors reported by GCC 10 -fanalyzer

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Hi,

On 5/11/20 3:07 PM, Nicolas Pomarède wrote:
> Le 11/05/2020 à 12:49, Eero Tamminen a écrit :
> We differ on their use, no problem. But for me, assert() is a "poor man" 
> fix. As I wrote, I understand assert() are put here and there while 
> developing new code, but years after, I think they should be replaced by 
> proper checks that write more detailled messages and maybe return 
> "false" or whatever code to indicate a function could not complete and 
> fix the problematic calling code at an upper level.

While asserts can be used liberally during
development to check things, often subset of them
is expected to remain in the code to make sure
future changes won't break things either.

Asserts can be also used as checks for correct API
usage (some languages provide real constraint
features, but in C people use assert).


> For example, I don't think the assert "assert(Drive >= 0 && Drive < 
> MAX_FLOPPYDRIVES)" in floppy.c are really needed. How could drive be >= 
> MAX_FLOPPYDRIVES, except if memory is already seriously corrupted ?

I seem to have added them in 2008 when adding new
features.  Feel free to remove those. :-)


> Exiting immediately doesn't seem a big benefit to me when compared to 
> crashing ; at least for the user, it looks the same.

It's *much* easier for user to copy paste the
assert print from console to a bug mail, than to
first install Hatari debug symbols (or re-compile
Hatari with symbols) and then use Gdb to find out
on which line the crash happens.


> And once again, the "problem" for me is not testing for "impossible" 
> situation, but thaassert() might be removed at compile time by different 
> compiler options ; so if we really want safeguards for "impossible" 
> situations, then better put our own code with some if/printf.

Asserts are something that are specifically made
to be easily removed from code with a define. So
is Hatari tracing.  Why that would be a problem?

GCC warnings from which this started, is separate
matter.  It's about NULL pointer checks for things
*where compiler knows that return value can be
NULL*, i.e. mallocs.  As already discussed, those
can and should use a wrapper.



	- Eero