| Message ID | GV1P250MB07370FFC43CBEB012B1CCB718F322@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,v2] configure: Explicitly check for static_assert | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Andreas Rheinhardt: > C11 provides static assertions via _Static_assert and > provides static_assert as a convenience define for this > in assert.h. MSVC 19.27 declares support for C11, but does > not support _Static_assert, but somehow supports > static_assert. That's therefore what we use. > > But apparently there are some old GCC toolchains where > _Static_assert is supported, but assert.h does not provide > the fallback define. Some fate boxes are affected by this > [1]. > > This commit therefore checks whether static_assert works > with assert.h included; if not, it errors out. Users like > the above can still add -Dstatic_assert=_Static_assert > to cflags as a workaround. > > [1]: https://fate.ffmpeg.org/report.cgi?time=20240321123620&slot=sh4-debian-qemu-gcc-4.7 > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > This is what a test without fallback looks like. > Posted to gather opinions on what people prefer. > > configure | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/configure b/configure > index 6d7b33b0ff..c2d2c70c20 100755 > --- a/configure > +++ b/configure > @@ -5589,6 +5589,19 @@ check_cxxflags_cc -std=$stdcxx ctype.h "__cplusplus >= 201103L" || > check_cflags_cc -std=$stdc ctype.h "__STDC_VERSION__ >= 201112L" || > { check_cflags_cc -std=c11 ctype.h "__STDC_VERSION__ >= 201112L" && stdc="c11" || die "Compiler lacks C11 support"; } > > +test_cc <<EOF || die "Compiler lacks support for C11 static_assert" > +#include <assert.h> > +#include <stddef.h> > +struct Foo { > + int a; > + void *ptr; > +} obj; > +static_assert(offsetof(struct Foo, a) == 0, > + "First element of struct does not have offset 0"); > +static_assert(offsetof(struct Foo, ptr) >= offsetof(struct Foo, a) + sizeof(obj.a), > + "elements not properly ordered in struct"); > +EOF > + > check_cppflags -D_FILE_OFFSET_BITS=64 > check_cppflags -D_LARGEFILE_SOURCE > Jan has tested old toolchains and found out that his GCC 4.7 has proper C11 headers; so this seems to be unique to Michael's setup. This makes me prefer this patch instead of the version with the fallback. (Michael can simply add -Dstatic_assert=_Static_assert to his cflags.) Of course others are still invited to share their opinions. - Andreas
On Thu, 21 Mar 2024, Andreas Rheinhardt wrote: > Andreas Rheinhardt: >> C11 provides static assertions via _Static_assert and >> provides static_assert as a convenience define for this >> in assert.h. MSVC 19.27 declares support for C11, but does >> not support _Static_assert, but somehow supports >> static_assert. That's therefore what we use. >> >> But apparently there are some old GCC toolchains where >> _Static_assert is supported, but assert.h does not provide >> the fallback define. Some fate boxes are affected by this >> [1]. >> >> This commit therefore checks whether static_assert works >> with assert.h included; if not, it errors out. Users like >> the above can still add -Dstatic_assert=_Static_assert >> to cflags as a workaround. >> >> [1]: https://fate.ffmpeg.org/report.cgi?time=20240321123620&slot=sh4-debian-qemu-gcc-4.7 >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> This is what a test without fallback looks like. >> Posted to gather opinions on what people prefer. >> >> configure | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/configure b/configure >> index 6d7b33b0ff..c2d2c70c20 100755 >> --- a/configure >> +++ b/configure >> @@ -5589,6 +5589,19 @@ check_cxxflags_cc -std=$stdcxx ctype.h "__cplusplus >= 201103L" || >> check_cflags_cc -std=$stdc ctype.h "__STDC_VERSION__ >= 201112L" || >> { check_cflags_cc -std=c11 ctype.h "__STDC_VERSION__ >= 201112L" && stdc="c11" || die "Compiler lacks C11 support"; } >> >> +test_cc <<EOF || die "Compiler lacks support for C11 static_assert" >> +#include <assert.h> >> +#include <stddef.h> >> +struct Foo { >> + int a; >> + void *ptr; >> +} obj; >> +static_assert(offsetof(struct Foo, a) == 0, >> + "First element of struct does not have offset 0"); >> +static_assert(offsetof(struct Foo, ptr) >= offsetof(struct Foo, a) + sizeof(obj.a), >> + "elements not properly ordered in struct"); >> +EOF >> + >> check_cppflags -D_FILE_OFFSET_BITS=64 >> check_cppflags -D_LARGEFILE_SOURCE >> > > Jan has tested old toolchains and found out that his GCC 4.7 has proper > C11 headers; so this seems to be unique to Michael's setup. This makes > me prefer this patch instead of the version with the fallback. (Michael > can simply add -Dstatic_assert=_Static_assert to his cflags.) > Of course others are still invited to share their opinions. Both patches seem to work fine with MSVC 19.27 - I vaguely prefer the v2 version, which is simpler. But to me, we could also just revert the change to libavcodec/ccaption_dec.c, and declare that we require MSVC 19.28 instead. MSVC 19.27, when executed with -std:c11 without -nologo, it prints this: /std:c11 is a preview implementation of the ISO C11 standard, and we're eager to hear about bugs and suggestions for improvements. However, note that these features are provided as-is without support. And I don't have any specific reasons for wanting to use this compiler - I just tested the lowest version that was supposed to be supported earlier and noted that it had broken recently. So to me, reverting to requiring _Static_assert would be quite ok as well. // Martin
Martin Storsjö: > On Thu, 21 Mar 2024, Andreas Rheinhardt wrote: > >> Andreas Rheinhardt: >>> C11 provides static assertions via _Static_assert and >>> provides static_assert as a convenience define for this >>> in assert.h. MSVC 19.27 declares support for C11, but does >>> not support _Static_assert, but somehow supports >>> static_assert. That's therefore what we use. >>> >>> But apparently there are some old GCC toolchains where >>> _Static_assert is supported, but assert.h does not provide >>> the fallback define. Some fate boxes are affected by this >>> [1]. >>> >>> This commit therefore checks whether static_assert works >>> with assert.h included; if not, it errors out. Users like >>> the above can still add -Dstatic_assert=_Static_assert >>> to cflags as a workaround. >>> >>> [1]: >>> https://fate.ffmpeg.org/report.cgi?time=20240321123620&slot=sh4-debian-qemu-gcc-4.7 >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>> --- >>> This is what a test without fallback looks like. >>> Posted to gather opinions on what people prefer. >>> >>> configure | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/configure b/configure >>> index 6d7b33b0ff..c2d2c70c20 100755 >>> --- a/configure >>> +++ b/configure >>> @@ -5589,6 +5589,19 @@ check_cxxflags_cc -std=$stdcxx ctype.h >>> "__cplusplus >= 201103L" || >>> check_cflags_cc -std=$stdc ctype.h "__STDC_VERSION__ >= 201112L" || >>> { check_cflags_cc -std=c11 ctype.h "__STDC_VERSION__ >= 201112L" >>> && stdc="c11" || die "Compiler lacks C11 support"; } >>> >>> +test_cc <<EOF || die "Compiler lacks support for C11 static_assert" >>> +#include <assert.h> >>> +#include <stddef.h> >>> +struct Foo { >>> + int a; >>> + void *ptr; >>> +} obj; >>> +static_assert(offsetof(struct Foo, a) == 0, >>> + "First element of struct does not have offset 0"); >>> +static_assert(offsetof(struct Foo, ptr) >= offsetof(struct Foo, a) + >>> sizeof(obj.a), >>> + "elements not properly ordered in struct"); >>> +EOF >>> + >>> check_cppflags -D_FILE_OFFSET_BITS=64 >>> check_cppflags -D_LARGEFILE_SOURCE >>> >> >> Jan has tested old toolchains and found out that his GCC 4.7 has proper >> C11 headers; so this seems to be unique to Michael's setup. This makes >> me prefer this patch instead of the version with the fallback. (Michael >> can simply add -Dstatic_assert=_Static_assert to his cflags.) >> Of course others are still invited to share their opinions. > > Both patches seem to work fine with MSVC 19.27 - I vaguely prefer the v2 > version, which is simpler. > > > But to me, we could also just revert the change to > libavcodec/ccaption_dec.c, and declare that we require MSVC 19.28 > instead. MSVC 19.27, when executed with -std:c11 without -nologo, it > prints this: > > /std:c11 is a preview implementation of the ISO C11 standard, and > we're eager to hear about bugs and suggestions for improvements. > However, note that these features are provided as-is without support. > > And I don't have any specific reasons for wanting to use this compiler - > I just tested the lowest version that was supposed to be supported > earlier and noted that it had broken recently. So to me, reverting to > requiring _Static_assert would be quite ok as well. > We can actually do both: Test for static_assert and for _Static_assert (to exclude MSVC 19.27; is 19.28 still supposed to be a preview implementation?). The reason I prefer static_assert in the codebase is that _Static_assert is actually deprecated with C23 (although I don't think it will be removed any time). - Andreas
On Fri, 22 Mar 2024, Andreas Rheinhardt wrote: > Martin Storsjö: >> >> Both patches seem to work fine with MSVC 19.27 - I vaguely prefer the v2 >> version, which is simpler. >> >> >> But to me, we could also just revert the change to >> libavcodec/ccaption_dec.c, and declare that we require MSVC 19.28 >> instead. MSVC 19.27, when executed with -std:c11 without -nologo, it >> prints this: >> >> /std:c11 is a preview implementation of the ISO C11 standard, and >> we're eager to hear about bugs and suggestions for improvements. >> However, note that these features are provided as-is without support. >> >> And I don't have any specific reasons for wanting to use this compiler - >> I just tested the lowest version that was supposed to be supported >> earlier and noted that it had broken recently. So to me, reverting to >> requiring _Static_assert would be quite ok as well. >> > > We can actually do both: Test for static_assert and for _Static_assert > (to exclude MSVC 19.27; is 19.28 still supposed to be a preview > implementation?). 19.28 no longer has that preview implementation banner, so from there on, it should be fine. > The reason I prefer static_assert in the codebase is that _Static_assert > is actually deprecated with C23 (although I don't think it will be > removed any time). Ah, I see. Right, with that in mind, unifying usage to static_assert sounds good. No strong opinion either way about the configure checks still (or whether we should require _Static_assert to be supported), except that strictly requiring static_assert seems less kludgy than trying to define it ourselves. // Martin
diff --git a/configure b/configure index 6d7b33b0ff..c2d2c70c20 100755 --- a/configure +++ b/configure @@ -5589,6 +5589,19 @@ check_cxxflags_cc -std=$stdcxx ctype.h "__cplusplus >= 201103L" || check_cflags_cc -std=$stdc ctype.h "__STDC_VERSION__ >= 201112L" || { check_cflags_cc -std=c11 ctype.h "__STDC_VERSION__ >= 201112L" && stdc="c11" || die "Compiler lacks C11 support"; } +test_cc <<EOF || die "Compiler lacks support for C11 static_assert" +#include <assert.h> +#include <stddef.h> +struct Foo { + int a; + void *ptr; +} obj; +static_assert(offsetof(struct Foo, a) == 0, + "First element of struct does not have offset 0"); +static_assert(offsetof(struct Foo, ptr) >= offsetof(struct Foo, a) + sizeof(obj.a), + "elements not properly ordered in struct"); +EOF + check_cppflags -D_FILE_OFFSET_BITS=64 check_cppflags -D_LARGEFILE_SOURCE
C11 provides static assertions via _Static_assert and provides static_assert as a convenience define for this in assert.h. MSVC 19.27 declares support for C11, but does not support _Static_assert, but somehow supports static_assert. That's therefore what we use. But apparently there are some old GCC toolchains where _Static_assert is supported, but assert.h does not provide the fallback define. Some fate boxes are affected by this [1]. This commit therefore checks whether static_assert works with assert.h included; if not, it errors out. Users like the above can still add -Dstatic_assert=_Static_assert to cflags as a workaround. [1]: https://fate.ffmpeg.org/report.cgi?time=20240321123620&slot=sh4-debian-qemu-gcc-4.7 Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- This is what a test without fallback looks like. Posted to gather opinions on what people prefer. configure | 13 +++++++++++++ 1 file changed, 13 insertions(+)