diff mbox

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

Message ID CAGn-TggU4cJxJM31brkc+q4SwRCO_dewu5WNn-e0Z_MrF_mkxA@mail.gmail.com
State New
Headers show

Commit Message

Adam Richter May 12, 2019, 12:49 p.m. UTC 
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

Comments

Michael Niedermayer May 13, 2019, 8:10 a.m. UTC | #1 
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@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@gmail.com>

LGTM

thx

[...]
diff mbox

Patch

From edb58a5ee8030ec66c04736a025d2a44e7322ba3 Mon Sep 17 00:00:00 2001
From: Adam Richter <adamrichter4@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@gmail.com>
---
 libavformat/au.c          | 3 ++-
 libavformat/avienc.c      | 3 ++-
 libavformat/aviobuf.c     | 3 ++-
 libavformat/matroskaenc.c | 6 ++++--
 libavformat/mov.c         | 3 ++-
 libavformat/rtmppkt.c     | 3 ++-
 libavformat/utils.c       | 9 +++++----
 7 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/libavformat/au.c b/libavformat/au.c
index cb48e67feb..76f23de56d 100644
--- a/libavformat/au.c
+++ b/libavformat/au.c
@@ -177,7 +177,8 @@  static int au_read_header(AVFormatContext *s)
             bps = 2;
         } else {
             const uint8_t bpcss[] = {4, 0, 3, 5};
-            av_assert0(id >= 23 && id < 23 + 4);
+            av_assert0(id >= 23);
+            av_assert0(id < 23 + 4);
             ba = bpcss[id - 23];
             bps = bpcss[id - 23];
         }
diff --git a/libavformat/avienc.c b/libavformat/avienc.c
index ac0f04c354..e96eb27e5e 100644
--- a/libavformat/avienc.c
+++ b/libavformat/avienc.c
@@ -797,7 +797,8 @@  static int avi_write_packet(AVFormatContext *s, AVPacket *pkt)
                 int pal_size = 1 << par->bits_per_coded_sample;
                 int pc_tag, i;
 
-                av_assert0(par->bits_per_coded_sample >= 0 && par->bits_per_coded_sample <= 8);
+                av_assert0(par->bits_per_coded_sample >= 0);
+                av_assert0(par->bits_per_coded_sample <= 8);
 
                 if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && avist->pal_offset) {
                     int64_t cur_offset = avio_tell(pb);
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index 5a33f82950..e71846e0e8 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -195,7 +195,8 @@  static void flush_buffer(AVIOContext *s)
 
 void avio_w8(AVIOContext *s, int b)
 {
-    av_assert2(b>=-128 && b<=255);
+    av_assert2(b >= -128);
+    av_assert2(b <= 255);
     *s->buf_ptr++ = b;
     if (s->buf_ptr >= s->buf_end)
         flush_buffer(s);
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index cef504fa05..51b6d1b16f 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -589,7 +589,8 @@  static int64_t mkv_write_cues(AVFormatContext *s, mkv_cues *cues, mkv_track *tra
             tracks[j].has_cue = 0;
         for (j = 0; j < cues->num_entries - i && entry[j].pts == pts; j++) {
             int tracknum = entry[j].stream_idx;
-            av_assert0(tracknum>=0 && tracknum<num_tracks);
+            av_assert0(tracknum >= 0);
+            av_assert0(tracknum < num_tracks);
             if (tracks[tracknum].has_cue && s->streams[tracknum]->codecpar->codec_type != AVMEDIA_TYPE_SUBTITLE)
                 continue;
             tracks[tracknum].has_cue = 1;
@@ -605,7 +606,8 @@  static int64_t mkv_write_cues(AVFormatContext *s, mkv_cues *cues, mkv_track *tra
             tracks[j].has_cue = 0;
         for (j = 0; j < cues->num_entries - i && entry[j].pts == pts; j++) {
             int tracknum = entry[j].stream_idx;
-            av_assert0(tracknum>=0 && tracknum<num_tracks);
+            av_assert0(tracknum >= 0);
+            av_assert0(tracknum < num_tracks);
             if (tracks[tracknum].has_cue && s->streams[tracknum]->codecpar->codec_type != AVMEDIA_TYPE_SUBTITLE)
                 continue;
             tracks[tracknum].has_cue = 1;
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 78f692872b..c45ea416fc 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -3267,7 +3267,8 @@  static void fix_index_entry_timestamps(AVStream* st, int end_index, int64_t end_
                                        int64_t* frame_duration_buffer,
                                        int frame_duration_buffer_size) {
     int i = 0;
-    av_assert0(end_index >= 0 && end_index <= st->nb_index_entries);
+    av_assert0(end_index >= 0);
+    av_assert0(end_index <= st->nb_index_entries);
     for (i = 0; i < frame_duration_buffer_size; i++) {
         end_ts -= frame_duration_buffer[frame_duration_buffer_size - 1 - i];
         st->index_entries[end_index - 1 - i].timestamp = end_ts;
diff --git a/libavformat/rtmppkt.c b/libavformat/rtmppkt.c
index 1eeae17337..04b651d45e 100644
--- a/libavformat/rtmppkt.c
+++ b/libavformat/rtmppkt.c
@@ -501,7 +501,8 @@  int ff_amf_tag_size(const uint8_t *data, const uint8_t *data_end)
     ret = amf_tag_skip(&gb);
     if (ret < 0 || bytestream2_get_bytes_left(&gb) <= 0)
         return -1;
-    av_assert0(bytestream2_tell(&gb) >= 0 && bytestream2_tell(&gb) <= data_end - data);
+    av_assert0(bytestream2_tell(&gb) >= 0);
+    av_assert0(bytestream2_tell(&gb) <= data_end - data);
     return bytestream2_tell(&gb);
 }
 
diff --git a/libavformat/utils.c b/libavformat/utils.c
index a63d71b0f4..efd1b17588 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -163,10 +163,11 @@  void av_format_inject_global_side_data(AVFormatContext *s)
 
 int ff_copy_whiteblacklists(AVFormatContext *dst, const AVFormatContext *src)
 {
-    av_assert0(!dst->codec_whitelist &&
-               !dst->format_whitelist &&
-               !dst->protocol_whitelist &&
-               !dst->protocol_blacklist);
+    av_assert0(!dst->codec_whitelist);
+    av_assert0(!dst->format_whitelist);
+    av_assert0(!dst->protocol_whitelist);
+    av_assert0(!dst->protocol_blacklist);
+
     dst-> codec_whitelist = av_strdup(src->codec_whitelist);
     dst->format_whitelist = av_strdup(src->format_whitelist);
     dst->protocol_whitelist = av_strdup(src->protocol_whitelist);
-- 
2.21.0