Re: [FFmpeg-devel] [PATCH 1/3] avutil/avassert: Add av_assert_unreachable()

[Michael Niedermayer <michael@niedermayer.cc>]

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

On Sat, Jul 13, 2024 at 04:44:03PM +0200, epirat07@gmail.com wrote:
> On 13 Jul 2024, at 2:47, Andreas Rheinhardt wrote:
> 
> > Marvin Scholz:
> >> ---
> >>  libavutil/avassert.h | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/libavutil/avassert.h b/libavutil/avassert.h
> >> index 1895fb7551..cdab912fe4 100644
> >> --- a/libavutil/avassert.h
> >> +++ b/libavutil/avassert.h
> >> @@ -75,4 +75,16 @@
> >>   */
> >>  void av_assert0_fpu(void);
> >>
> >> +/**
> >> + * Assert this can not be reached
> >> + */
> >> +#if AV_HAS_BUILTIN(__builtin_unreachable)
> >> +#define av_assert_unreachable() do {   \
> >> +    av_assert2(0);                      \
> >> +    __builtin_unreachable();            \
> >> +} while (0)
> >> +#else
> >> +#define av_assert_unreachable() av_assert2(0)
> >> +#endif
> >> +
> >>  #endif /* AVUTIL_AVASSERT_H */
> >>
> >> base-commit: 85706f5136cf7c88f95843b2634dd3f7d7d2cb6d
> >
> > You are not the first one with this idea:
> > https://ffmpeg.org/pipermail/ffmpeg-devel/2024-May/328116.html
> > But Michael Niedermayer thinks that adding a new macro instead of
> > directly reusing av_assert is more complicated.
> >
> Oh I did not notice that one, thanks…
> 
> I definitely think an av_assert_unreachable() is clearer than
> av_assert0(!"reached") too, obviously, else I would not propose it.
> 
> (Somewhat proven by these two cases fixed by the patch where we use
> av_assert1/2(0) variants which then result in the compiler not knowing
> these are unreachable cases when the assert is not enabled.)
> 
> If this approach is rejected, I can submit a version that uses
> av_assert0(!"reached“).

I think you look at this from the wrong side

We have 3 levels of assert()
and we have one threshold to decide how much we want to check at runtime

what we do NOT have is a threshold at what we want the compiler to assume
teh statment is true
And here this av_assert1(0) is not assumed true and not checked at runtime
so the compiler thinks this is reachable

I think the solution is to add this 2nd threshold so we can gloablly
use av_assert*() for optimization
This should remove the warning with no changes to the code

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates

[-- 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".