[FFmpeg-devel] [PATCH] libavformat: Separate assertions of the form "av_assertN(a && b)" to "av_assertN(a); av_assertN(b)" for more precise diagnostics.

[Michael Niedermayer]

On Sun, May 12, 2019 at 05:49:00AM -0700, Adam Richter wrote:
> This is the first of what I expect to be several patches to convert
> assertions of the
> form "assert(a && b)" to "assert(a); assert(b)".
> 
> Here are some reasons why I think this would be an improvement.  This
> lengthy argument is not included in the patch attachment.
> 
> 1. Assertion failures are often sporadic, and users who report them may
>    not be in a position to efficiently narrow them down further, so it
>    is important to get as much precision from each assertion failure as
>    possible.
> 
> 2. It is a more efficient use of developers time when a bug report
>    arrives if the problem has been narrowed down that much more.  A
>    new bug report may initially attract more interest, so, if the
>    problem has been narrowed down that much more, it may increase the
>    chance that developers may invest the time to try to resolve the
>    problem, and also reduce unnecessary traffic on the developer mailing
>    list about possible causes of the bug that separating the assertion
>    was able to rule out.
> 
> 3. It's often more readable, sometimes eliminating parentheses or
>    changing multi-line conditions to separate single line conditions.
> 
> 4. When using a debugger to step over an assertion failure in the
>    first part of the statement, the second part is still tested.
> 
> 5. Providing separate likelihood hints to the compiler in the form
>    of separate assert statements does not require the compiler to
>    be quite as smart to recognize that it should optimize both branches,
>    although I do not know if that makes a difference for any compiler
>    commonly used to compile X (that is, I suspect that they are all
>    smart enough to realize is that "a && b" is likely true, then "a"
>    is likely true and "b" is likely true).
> 
> I have confirmed that the resulting tree built without any apparent
> complaints about the assert statements and that "make fate" completed
> with a zero exit code.  I have not done any other tests though.
> 
> Thanks in advance for considering this patch.
> 
> Adam

>  au.c          |    3 ++-
>  avienc.c      |    3 ++-
>  aviobuf.c     |    3 ++-
>  matroskaenc.c |    6 ++++--
>  mov.c         |    3 ++-
>  rtmppkt.c     |    3 ++-
>  utils.c       |    9 +++++----
>  7 files changed, 19 insertions(+), 11 deletions(-)
> c36df9add7cb81a670d3e2ca2bbd7ee20d25cc51  0001-libavformat-Separate-assertions-of-the-form-av_asser.patch
> From edb58a5ee8030ec66c04736a025d2a44e7322ba3 Mon Sep 17 00:00:00 2001
> From: Adam Richter <adamrichter4 at gmail.com>
> Date: Sun, 12 May 2019 03:41:49 -0700
> Subject: [PATCH] libavformat: Separate assertions of the form
>  "av_assertN(a && b)" to "av_assertN(a); av_assertN(b)" for more precise
>  diagnostics.
> 
> Signed-off-by: Adam Richter <adamrichter4 at gmail.com>

LGTM

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- 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/20190513/33df78c6/attachment.sig>