[MPlayer-dev-eng] [PATCH] vf_delogo: variable rectangle

[Reimar Döffinger]

On 18 May 2011, at 13:16, Nicolas George <nicolas.george at normalesup.org> wrote:
> Le nonidi 29 floréal, an CCXIX, Reimar Döffinger a écrit :
>> I still dislike the mp_mem all-around: I do not like those hard aborts, I
> 
> Maybe you did not look at the latest version: there are no longer hard
> aborts but rather clean shutdowns using exit_mplayer().

I considered exit_mplayer a "hard abort".
And I can't quite decide whether it is better or worse: If someone ever used the function in code that somehow might be called by exit_mplayer you can now get an endless recursion.

> 
>> do not like adding a new file with "generic" code that will be used only
>> in one place (with a risk that this never changes, making it a useless
>> maintenance burden)
> 
> I agree this is not ideal, but I believe that adding the code to do the
> error checks as is in place is much worse: we end up with each place needing
> malloc doing things its ways, sometimes with size_t and sometimes with int,
> sometimes checking for overflow, sometimes only for failure, and sometimes
> for neither, etc.

I already said that I am quite in favour of code like realloc_struct handling overflows.
However the failure handling is typical dead-end code: though in principle sensible, it is still a lazy hack - good code would return error codes up the chain until a point where the error can be handled, e.g. by just going on to play the next file - and "good code" will not be able to use a function with built-in exit.

>>              and I don't really like that the realloc integer
>> overflow check is not done in the usual way (nelem > MAX / element_size),
>> even though I at least currently can't think of a case where it would
>> fail.
> 
> I am pretty sure this version is correct,

Usually (theoretical) issues come up once signed types become involved, haven't checked for that.

> and I quite like the fact that it
> works even if SIZE_MAX is missing or wrong or if the type of the variable
> changes,

Some arbitrary value like INT_MAX (or rather some fixed constant that will always fit in an int) also has the advantage that behaviour will be the same on all systems.
But either way I think there has been enough pointless discussion. I sure think that reusing the existing function is better (since it is tested and has several users), and that exiting on failure has no real advantages but has several issues including discouraging really good code but if I can't convince you just go ahead.