[FFmpeg-devel] [PATCH 001/244] Add a new channel layout API

[Anton Khirnov]

Quoting Nicolas George (2019-12-17 20:20:00)
> Hi.
> 
> Anton Khirnov (12019-12-10):
> > I disagree. I think I have a fair amount of skill and experience as a C
> > developer, but I still get hit by those issues frequently. It's extra
> > trouble for only theoretical gain.
> 
> I concede this to you, except for the last sentence: I do not agree that
> the gain is only theoretical. We only need to look at the number of
> recent commits fixing possible integer overflows. Signed arithmetic is
> tricky, possibly as tricky as the traps of mixing signed and unsigned.
> The reason we are seeing this only now is because compilers have only
> recently started to optimize these undefined behaviors aggressively. And
> we have only recently started to care. But these are real traps that do
> not happen with unsigned.

Maybe I missed something, but I am not aware of the UB-ness of signed
overflow being a practical problem. Typically, your computation will
return a meaningless result. You would get a similarly meaningless
result from the analogous perfectly well-defined unsigned computation.

to be clear: I am not objecting against fixing UB, but clarifying my
'theoretical gain' comment above.

> 
> I would advise everybody to adopt the policy of always using unsigned
> unless there is a good reason to use signed, instead of the default now
> which is the opposite.
> 
> Of course, for your own code and everybody else, I would not block the
> patch if my advice is not followed. But this is public API, it should be
> clean, so I will insist a little more.

I would agree to that only if it was done consistently across the entire
API. Since the channel count tends to be used in expressions with other
values which are signed (e.g. sample rate). Which means that the users
will have to mix signed and unsigned, making things harder for them.

> 
> Using the logical type has practical benefits. One is that it guarantees
> to the API users that negative values cannot happen, need not to be
> tested. With an unsigned, the value can be injected in size and pointer
> arithmetic directly. With a signed, the API user need either to add an
> useless test or to blindly trust the library. Blindly trusting a piece
> of code should always put people ill at ease.

The guarantee that it cannot be negative in valid cases is already
present, there is no need for users to test that explicitly. The only
conceivable case where it will be negative is a bug in the code
populating the channel layout. Making the value unsigned would not make
such bugs go away, it would only make the value absurdly large instead
of negative.

-- 
Anton Khirnov