On Sun, May 12, 2019 at 11:16 AM Michael Niedermayer <mich...@niedermayer.cc> wrote: > > 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 [...]
Hello, Michael and Mark. Thank you for your reviews of my proposed changes that would split most assertions of the form "a && b" into separate assertions. Michael, if I understand correctly, you are OK with the patches after having looked at them to spot problems, but are not advocating for or against their inclusion. Mark, if I understand correctly, you would rather see assertions split only where "a and b are not related." I don't think anyone else has chimed in, but I would welcome being told if I missed anyone else's expressed opinion or if anyone wants to add anything now. To make the most efficient use of your time, I would like to proceed as follows. In the rest of this message, I will include an argument, primarily in the hopes of persuading you, Mark. After that, if you're still not on board with merging all of the patches, and if there are no other replies in the next day or two, then I encourage you to post a subset of my patches which you think should be merged (if any, and if you have the time). Otherwise, probably around this weekend, I will try to make a new subset of these patches that I think you might be OK with and post that for comments. However, first, in the hopes that we can avoid picking out which part of the patches to merge, here is a response, Mark, to your previously stated objections. As a preliminary, regarding your remark "Assertions are primarily present to assist a developer reading/writing the code", I just want to clarify something for other readers that, av_assert0 statements are always compiled in, and, as for the other assertions, there may be some small value in reducing their costs so that they result in timing behavior that is a bit closer to the behavior without the assertions, which might be where a bug occurred. So, there may still be some small value in being able to pass branch predictions to the compiler in the assertion statement, although I was surprised to see that neither glibc nor ffmpeg appear currently to do this. However, I recognize that most of your argument, Mark, is about readability. At least in my limited experience, I think that, separated assertions are actually almost always more readable, sometimes eliminating parentheses, often making references to the same values line up on the same columns, which can make range checks more obvious, and sometimes changing multi-line conditions to separate single line conditions, which can be recognized separately. Consider, for example, if you agree that columnization makes this range check more recognizable in a glance and makes it easier to spot what the bounds are (the sixteen space indentation is taken from the code in which it appeared): av_assert0(par->bits_per_coded_sample >= 0 && par->bits_per_coded_sample <= 8); ...vs... av_assert0(par->bits_per_coded_sample >= 0); av_assert0(par->bits_per_coded_sample <= 8); A possible counter-argument to this might be that, in a long sequence of assertions, can be informative to group related assertions together, which I think is true, but it is possible to get this other means, such as by using blank lines to separate express such grouping. So, Mark, if you decide you are OK with my complete patches, I encourage you to let me know. Otherwise, if there are any of those changes which you are OK with, I would like to just to to get those merged. Finally, if you would rather see none of those changes merged (one one to split the assertions in libavformat and one was for everywhere else in ffmpeg), please let me know about that too, in which case, if no one advocates for their inclusion, I'll drop my proposal to merge these changes. By the way, after this, regarding the your (Mark and Michael's) discussion of changing the few assert() call that my patches effected to av_assertN, I have not looked in much depth, but I think that all such cases may be in pretty busy paths, so it might be most appropriate to replace them with av_assert2, which normally is not compiled in. Also after this, I may take a look at adding a branch hint to the av_assertN macros if nobody objects. In any case, thanks for your thoughtful consideration of these proposed edits. Adam _______________________________________________ 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".