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 <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().
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
signature.asc
Description: PGP signature
_______________________________________________ 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".