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 <adamricht...@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 <adamricht...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
