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

[Michael Niedermayer]

On Sun, May 12, 2019 at 05:58:50PM +0100, Mark Thompson wrote:
> 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().

I agree that they should be changed to av_assertN() but it should be
in a seperate commit/patch

These assert -> av_assertN changes will likely in some cases "activate"
"dormant" checks which fail. Managing / Testing / Reviewing and correcting
these should be easier if they are not in a large change splitting asserts

Thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"You are 36 times more likely to die in a bathtub than at the hands of a
terrorist. Also, you are 2.5 times more likely to become a president and
2 times more likely to become an astronaut, than to die in a terrorist
attack." -- Thoughty2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190512/ac507e4e/attachment.sig>