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

[mypopy at gmail.com]

On Wed, May 15, 2019 at 7:01 AM Hendrik Leppkes <h.leppkes at gmail.com> wrote:
>
> On Tue, May 14, 2019 at 11:25 PM Adam Richter <adamrichter4 at gmail.com> wrote:
> >
> > 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.
> >
>
> Unfortunately I have to agree with Mark. asserst that check the same
> value or extremely closely related values should stay combined.
>
I agree this part
> >
> > Also after this, I may take a look at adding a branch hint to the av_assertN
> > macros if nobody objects.
> >
>
> Please don't, we don't do branch hints anywhere, and this is a bad
> place to start.
> If an assert is truely performance sensitive (as in, it makes a
> measurable difference), it should probably be moved from assert0 to
> assert1, and as such is only enabled in testing builds.
>
If assert0 or assert1 lead to performance drop, we need some profiling
data, then try to fix it.
> - Hendrik