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

[ Thread Index | Date Index | More Archives ]

Le 11/05/2020 à 12:49, Eero Tamminen a écrit :

On 5/11/20 12:53 PM, Nicolas Pomarède wrote:
Le 11/05/2020 à 11:36, Eero Tamminen a écrit :
The point of asserts is to show places where logic
errors happen. Malloc failure isn't a logic error,
so I think hiding the error location with
a wrapper is fine.

I think logic errors are better handled by a real printf + possible return value to try to continue anyway instead of exiting the program.

By logic error I mean "this code didn't work as
it should, all bets are now off".  You shouldn't
continue at that point, after that point things
will get only progressively harder to debug.

If assert is on a condition that can happen and
from which you can recover, you shouldn't use an

If someone is not sure about the logic of his code, assert() can be used, but IMO they should be removed later when logic is fixed. Keeping assert() in years old code in case logic is still wrong looks bad programing to me :) (it's not about the few bytes of code assert adds).

They are there to guard against new changes
accidentally breaking old code.  Years later you
might not anymore remember exactly how it was
supposed to work.  For example profiling call
graph collection is of that complexity.

Assert could in theory be triggered also by
something (e.g. array overwrite) corrupting
Hatari's internal state.

You could think of an assert as a unit test.
You don't remove unit tests after they stop

Also, how do we know these assert() won't be removed by some linux distro or windows/mac binaries, depending on global compiler flags we don't control ? And instead of catching a possible error with assert(), the program will just continue and maybe crash later.

Asserts shouldn't be used for a potential errors,
but for "can't happen" things.  While they should
be enabled for all development builds, disabling
them for releases could be OK as code changes in
those should have already been tested.

In releases they might still catch memory corruption issues though, e.g. due to different
build environments, so it would still be
preferable for them to be enabled in releases.

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.

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 ?

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

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.


Mail converted by MHonArc 2.6.19+