| Message ID | D2NRO3OJ5GVS.1127UPMI9AZZY@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avutil/avassert: Add av_assert_unreachable() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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. - Andreas
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“). > - Andreas > > _______________________________________________ > 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".
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 [...]
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 */