Re: [FFmpeg-devel] [PATCH 01/12] avutil/avassert: Add av_unreachable and av_assume() macros

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 2948 bytes --]

Hi

On Fri, May 24, 2024 at 11:58:21PM +0200, Andreas Rheinhardt wrote:
> Useful to let the compiler and static analyzers know that
> something is unreachable without adding an av_assert
> (which would be either dead for the compiler or add runtime
> overhead) for this.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> I can add more macros if it is desired to differentiate between
> ASSERT_LEVEL == 1 and ASSERT_LEVEL > 1.
> 
>  doc/APIchanges       |  3 +++
>  libavutil/avassert.h | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 60f056b863..5a3ae37999 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2024-03-07
>  
>  API changes, most recent first:
>  
> +2024-05-24 - xxxxxxxxxx - lavu 59.xx.100 - avassert.h
> +  Add av_unreachable and av_assume() macros.
> +
>  2024-05-23 - xxxxxxxxxx - lavu 59.20.100 - channel_layout.h
>    Add av_channel_layout_ambisonic_order().
>  
> diff --git a/libavutil/avassert.h b/libavutil/avassert.h
> index 1895fb7551..41e29c7687 100644
> --- a/libavutil/avassert.h
> +++ b/libavutil/avassert.h
> @@ -31,6 +31,7 @@
>  #ifdef HAVE_AV_CONFIG_H
>  #   include "config.h"
>  #endif
> +#include "attributes.h"
>  #include "log.h"
>  #include "macros.h"
>  
> @@ -68,6 +69,38 @@
>  #define av_assert2_fpu() ((void)0)
>  #endif
>  
> +/**
> + * Asserts that are used as compiler optimization hints depending
> + * upon ASSERT_LEVEL and NBDEBUG.
> + *
> + * Undefined behaviour occurs if execution reaches a point marked
> + * with av_unreachable or if a condition used with av_assume()
> + * is false.
> + *
> + * The condition used with av_assume() should not have side-effects
> + * and should be visible to the compiler.
> + */

this feels wrong

We have 3 assert functions

one for security relevant code or other things we always want to check and not play around

one for speed relevant code where we dont want to check in production code. But may want
to do checks if we are debuging.

and one for the cases between


What is an assert ? Its a statement about a condition that is true unless the code
is broken. Its never correct to use an assert to check for a condition that is known
to be false for some input.
So a assert really already is either

A. Check, print, abort
or
B. undefined if false

But if an assert already is "undefined if false" then what you add is not
usefull, just add the compiler specific "assume" code to the disabled asserts

This would also keep the API simpler

thx

[..]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".