[FFmpeg-devel] [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more precise diagnostics, except for libformat

[Mark Thompson]

On 12/05/2019 16:24, Adam Richter wrote:
> This patch separates statements of the form "assert(a && b);" into
> "assert(a);" and "assert(b);", typically involving an assertion
> function like av_assert0.
> 
> This patch covers all of ffmpeg, except for the libavformat, which I
> have already submitted separately.
> 
> I have not tested this patch other than observing that ffmpeg still
> builds without any apparent new complaints, that no complaints in the
> build contain "assert", and that "make fate" seems to succeed.
> 
> Thanks in advance for considering the attached patch.
> 
> Adam
> 
> 
> From f815a2481a19cfd191b9f97e246b307b71d6c790 Mon Sep 17 00:00:00 2001
> From: Adam Richter <adamrichter4 at gmail.com>
> Date: Sun, 12 May 2019 08:02:51 -0700
> Subject: [PATCH] "assert(a && b)" --> "assert(a); assert(b)" for more
>  precise diagnostics, except for libformat.
> 
> This patch separates statements of the form "assert(a && b);" into
> "assert(a);" and "assert(b);", for more precise diagnostics when
> an assertion fails, which can be especially important in cases where
> the problem may not be easily reproducible and save developer time.
> Usually, this involves functions like av_assert0.

I don't feel very convinced by the general case of this argument.  Assertions are primarily present to assist a developer reading/writing the code; they should never be triggering at runtime in non-development contexts.

Where the statements a and b are not related then yes, splitting them is a good idea.  But when it's something like a bounds check on one variable ("av_assert0(A < n && n < B)", as appears quite a few times below) then I think keeping it as one statement feels clearer.  Similarly for highly related conditions ("av_assert0(p && p->f)" or "av_assert0(x < width && y < height)").

> There are a couple of cases that this patch does not change:
> (1) assert conjunctions of the form assert(condition && "string literal
>     to pass to the user as a helpful tip."), and
> (2) assert condjunctions where the first part contained a variable
>     assignment that was used in the second part of the assertion and
>     not outside the assert (so that the variable assignment be elided
>     if the assertion checking omitted).
> 
> This patch covers all of ffmpeg except for libavformat, which was
> covered in a patch that I previously submitted separately.
> 
> These changes build without any new complaints that I noticed, and
> "make fate" succeeds, but I have not otherwise tested them.
> 
> Signed-off-by: Adam Richter <adamrichter4 at gmail.com>
> ...
> diff --git a/libavcodec/aacpsy.c b/libavcodec/aacpsy.c
> index fca692cb15..bef52e8b02 100644
> --- a/libavcodec/aacpsy.c
> +++ b/libavcodec/aacpsy.c
> @@ -504,9 +504,11 @@ static int calc_bit_demand(AacPsyContext *ctx, float pe, int bits, int size,
>      fill_level = av_clipf((float)ctx->fill_level / size, clip_low, clip_high);
>      clipped_pe = av_clipf(pe, ctx->pe.min, ctx->pe.max);
>      bit_save   = (fill_level + bitsave_add) * bitsave_slope;
> -    assert(bit_save <= 0.3f && bit_save >= -0.05000001f);
> +    assert(bit_save <= 0.3f);
> +    assert(bit_save >= -0.05000001f);
>      bit_spend  = (fill_level + bitspend_add) * bitspend_slope;
> -    assert(bit_spend <= 0.5f && bit_spend >= -0.1f);
> +    assert(bit_spend <= 0.5f);
> +    assert(bit_spend >= -0.1f);

While you're touching calls to traditional assert() please consider turning them into suitable av_assertN().

- Mark