| Message ID | BANLkTim7utDK7kQn-zKUGyF8QTMMF4Sqog@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Any chance to get this reviewed/applied any time soon ? It currently does not build without it with gcc 4.6.0 On 29 April 2011 17:59, Marc-Antoine Perennou <Marc-Antoine@perennou.com> wrote: > pulse/simple.h does not include stdlib.h > We cannot use NULL since it may not be defined > Use 0 instead > > Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com> > --- > configure | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/configure b/configure > index ea8b676..d67c3ce 100755 > --- a/configure > +++ b/configure > @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do > > pa) > audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \ > - "pa_simple *s = NULL; pa_simple_free(s); return 0;" > + "pa_simple *s = 0; pa_simple_free(s); return 0;" > libs_softmmu="-lpulse -lpulse-simple $libs_softmmu" > audio_pt_int="yes" > ;; > -- > 1.7.5.52.ge839f.dirty >
On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote: > pulse/simple.h does not include stdlib.h > We cannot use NULL since it may not be defined > Use 0 instead I am unable to reproduce this issue, even with gcc-4.6. Also please note that NULL is defined in <stddef.h>, not <stdlib.h>. <stddef.h> is included from <sys/types.h> which is included from <pulse/simple.h>. Do you have more information about the issue. > Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com> > --- > configure | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/configure b/configure > index ea8b676..d67c3ce 100755 > --- a/configure > +++ b/configure > @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do > > pa) > audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \ > - "pa_simple *s = NULL; pa_simple_free(s); return 0;" > + "pa_simple *s = 0; pa_simple_free(s); return 0;" It should be ((void*)0) instead of simply 0. > libs_softmmu="-lpulse -lpulse-simple $libs_softmmu" > audio_pt_int="yes" > ;; > -- > 1.7.5.52.ge839f.dirty > >
On Fri, 3 Jun 2011, Aurelien Jarno wrote: > On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote: > > pulse/simple.h does not include stdlib.h > > We cannot use NULL since it may not be defined > > Use 0 instead > > I am unable to reproduce this issue, even with gcc-4.6. Also please note > that NULL is defined in <stddef.h>, not <stdlib.h>. <stddef.h> is > included from <sys/types.h> which is included from <pulse/simple.h>. > > Do you have more information about the issue. > > > Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com> > > --- > > configure | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/configure b/configure > > index ea8b676..d67c3ce 100755 > > --- a/configure > > +++ b/configure > > @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do > > > > pa) > > audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \ > > - "pa_simple *s = NULL; pa_simple_free(s); return 0;" > > + "pa_simple *s = 0; pa_simple_free(s); return 0;" > > It should be ((void*)0) instead of simply 0. No it shouldn't. > > > libs_softmmu="-lpulse -lpulse-simple $libs_softmmu" > > audio_pt_int="yes" > > ;; > > -- > > 1.7.5.52.ge839f.dirty > > > > > >
On Sat, Jun 04, 2011 at 01:57:23AM +0400, malc wrote: > On Fri, 3 Jun 2011, Aurelien Jarno wrote: > > > On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote: > > > pulse/simple.h does not include stdlib.h > > > We cannot use NULL since it may not be defined > > > Use 0 instead > > > > I am unable to reproduce this issue, even with gcc-4.6. Also please note > > that NULL is defined in <stddef.h>, not <stdlib.h>. <stddef.h> is > > included from <sys/types.h> which is included from <pulse/simple.h>. > > > > Do you have more information about the issue. > > > > > Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com> > > > --- > > > configure | 2 +- > > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > > > diff --git a/configure b/configure > > > index ea8b676..d67c3ce 100755 > > > --- a/configure > > > +++ b/configure > > > @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do > > > > > > pa) > > > audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \ > > > - "pa_simple *s = NULL; pa_simple_free(s); return 0;" > > > + "pa_simple *s = 0; pa_simple_free(s); return 0;" > > > > It should be ((void*)0) instead of simply 0. > > No it shouldn't. > If we want to replace #include <stddef.h>, which should use the same code, that is: | #ifndef __cplusplus | #define NULL ((void *)0) | #else /* C++ */ | #define NULL 0 | #endif /* C++ */ Given we are writing C code and not C++ code, NULL is defined as ((void *)0).
On Sat, 4 Jun 2011, Aurelien Jarno wrote: > On Sat, Jun 04, 2011 at 01:57:23AM +0400, malc wrote: > > On Fri, 3 Jun 2011, Aurelien Jarno wrote: > > > > > On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote: > > > > pulse/simple.h does not include stdlib.h > > > > We cannot use NULL since it may not be defined > > > > Use 0 instead > > > > > > I am unable to reproduce this issue, even with gcc-4.6. Also please note > > > that NULL is defined in <stddef.h>, not <stdlib.h>. <stddef.h> is > > > included from <sys/types.h> which is included from <pulse/simple.h>. > > > > > > Do you have more information about the issue. > > > > > > > Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com> > > > > --- > > > > configure | 2 +- > > > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > > > > > diff --git a/configure b/configure > > > > index ea8b676..d67c3ce 100755 > > > > --- a/configure > > > > +++ b/configure > > > > @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do > > > > > > > > pa) > > > > audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \ > > > > - "pa_simple *s = NULL; pa_simple_free(s); return 0;" > > > > + "pa_simple *s = 0; pa_simple_free(s); return 0;" > > > > > > It should be ((void*)0) instead of simply 0. > > > > No it shouldn't. > > > > If we want to replace #include <stddef.h>, which should use the same > code, that is: > > | #ifndef __cplusplus > | #define NULL ((void *)0) > | #else /* C++ */ > | #define NULL 0 > | #endif /* C++ */ > > Given we are writing C code and not C++ code, NULL is defined as > ((void *)0). Doesn't make a dent of a differnce how particular implementation defines NULL, 6.2.3.3#3 covers it, what i'm trying to get across is that your strong "should" is wrong.
On 4 June 2011 00:54, malc <av1474@comtv.ru> wrote: > On Sat, 4 Jun 2011, Aurelien Jarno wrote: > >> On Sat, Jun 04, 2011 at 01:57:23AM +0400, malc wrote: >> > On Fri, 3 Jun 2011, Aurelien Jarno wrote: >> > >> > > On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote: >> > > > pulse/simple.h does not include stdlib.h >> > > > We cannot use NULL since it may not be defined >> > > > Use 0 instead >> > > >> > > I am unable to reproduce this issue, even with gcc-4.6. Also please note >> > > that NULL is defined in <stddef.h>, not <stdlib.h>. <stddef.h> is >> > > included from <sys/types.h> which is included from <pulse/simple.h>. >> > > >> > > Do you have more information about the issue. >> > > >> > > > Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com> >> > > > --- >> > > > configure | 2 +- >> > > > 1 files changed, 1 insertions(+), 1 deletions(-) >> > > > >> > > > diff --git a/configure b/configure >> > > > index ea8b676..d67c3ce 100755 >> > > > --- a/configure >> > > > +++ b/configure >> > > > @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do >> > > > >> > > > pa) >> > > > audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \ >> > > > - "pa_simple *s = NULL; pa_simple_free(s); return 0;" >> > > > + "pa_simple *s = 0; pa_simple_free(s); return 0;" >> > > >> > > It should be ((void*)0) instead of simply 0. >> > >> > No it shouldn't. >> > >> >> If we want to replace #include <stddef.h>, which should use the same >> code, that is: >> >> | #ifndef __cplusplus >> | #define NULL ((void *)0) >> | #else /* C++ */ >> | #define NULL 0 >> | #endif /* C++ */ >> >> Given we are writing C code and not C++ code, NULL is defined as >> ((void *)0). > > Doesn't make a dent of a differnce how particular implementation defines > NULL, 6.2.3.3#3 covers it, what i'm trying to get across is that your > strong "should" is wrong. > > -- > mailto:av1474@comtv.ru It's kinda weird actually, just wrote a small program to test: #include <stddef.h> int main() { int * i = NULL; return 0; } It builds fine Now if I replace "stddef.h" by "sys/types.h" ‘NULL’ undeclared I'm running Gentoo GNU/Linux, and everyone using it in my friends get the same problem, and there is no problem at all with the whole system which is around. (Only gcc 4.6 is installed fyi) Manually including stddef.h or replacing NULL by 0 or (void*)0 makes it work. Hope I provided enough informations to justify this thread
Am 09.06.2011 um 17:52 schrieb Marc-Antoine Perennou: > On 4 June 2011 00:54, malc <av1474@comtv.ru> wrote: >> On Sat, 4 Jun 2011, Aurelien Jarno wrote: >> >>> On Sat, Jun 04, 2011 at 01:57:23AM +0400, malc wrote: >>>> On Fri, 3 Jun 2011, Aurelien Jarno wrote: >>>> >>>>> On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou >>>>> wrote: >>>>>> diff --git a/configure b/configure >>>>>> index ea8b676..d67c3ce 100755 >>>>>> --- a/configure >>>>>> +++ b/configure >>>>>> @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do >>>>>> >>>>>> pa) >>>>>> audio_drv_probe $drv pulse/simple.h "-lpulse-simple - >>>>>> lpulse" \ >>>>>> - "pa_simple *s = NULL; pa_simple_free(s); return 0;" >>>>>> + "pa_simple *s = 0; pa_simple_free(s); return 0;" > It's kinda weird actually, just wrote a small program to test: > > #include <stddef.h> > int main() { > int * i = NULL; > return 0; > } > > It builds fine > Now if I replace "stddef.h" by "sys/types.h" > ‘NULL’ undeclared > > I'm running Gentoo GNU/Linux [...] > > Manually including stddef.h or replacing NULL by 0 or (void*)0 makes > it work. Then please submit a new patch that explicit includes that header with a comment explaining why, so that it doesn't get removed by accident. Andreas
Aurelien Jarno <aurelien@aurel32.net> writes: > On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote: >> pulse/simple.h does not include stdlib.h >> We cannot use NULL since it may not be defined >> Use 0 instead > > I am unable to reproduce this issue, even with gcc-4.6. Also please note > that NULL is defined in <stddef.h>, not <stdlib.h>. NULL is defined in stddef.h (ISO/IEC 9899:1999 7.17), locale.h (ibid 7.11), stdio.h (ibid 7.19.1), stdlib.h (ibid 7.20), string.h (ibid 7.21.1), time.h (ibid 7.23.1), wchar.h (ibid 7.24.1). > <stddef.h> is > included from <sys/types.h> which is included from <pulse/simple.h>. I don't think POSIX specifies sys/types.h defines NULL, let alone includes stddef.h. > Do you have more information about the issue. > >> Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com> >> --- >> configure | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/configure b/configure >> index ea8b676..d67c3ce 100755 >> --- a/configure >> +++ b/configure >> @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do >> >> pa) >> audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \ >> - "pa_simple *s = NULL; pa_simple_free(s); return 0;" >> + "pa_simple *s = 0; pa_simple_free(s); return 0;" > > It should be ((void*)0) instead of simply 0. Matter of taste. >> libs_softmmu="-lpulse -lpulse-simple $libs_softmmu" >> audio_pt_int="yes" >> ;;
On 9 June 2011 18:44, Andreas Färber <andreas.faerber@web.de> wrote: > Am 09.06.2011 um 17:52 schrieb Marc-Antoine Perennou: >> Manually including stddef.h or replacing NULL by 0 or (void*)0 makes it >> work. > > Then please submit a new patch that explicit includes that header with a > comment explaining why, so that it doesn't get removed by accident. I think the original patch is fine. This configure test isn't trying to test whether we correctly found some definition of NULL, it's testing pulseaudio presence. The test code should be the simplest and most stand-alone code that achieves that. There's no need to go dragging in extra headers when we don't even need to be using NULL here anyhow. We already have configure tests which use unadorned 0 for pointers (eg the pthread test), it's legal C, and it works. I think we should call this a trivial patch and just apply it. Reviewed-by: Peter Maydell <peter.maydell@linaro.org> -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 9 June 2011 18:44, Andreas Färber <andreas.faerber@web.de> wrote: >> Am 09.06.2011 um 17:52 schrieb Marc-Antoine Perennou: >>> Manually including stddef.h or replacing NULL by 0 or (void*)0 makes it >>> work. >> >> Then please submit a new patch that explicit includes that header with a >> comment explaining why, so that it doesn't get removed by accident. > > I think the original patch is fine. This configure test isn't > trying to test whether we correctly found some definition of > NULL, it's testing pulseaudio presence. The test code should > be the simplest and most stand-alone code that achieves that. > There's no need to go dragging in extra headers when we don't > even need to be using NULL here anyhow. > > We already have configure tests which use unadorned 0 for > pointers (eg the pthread test), it's legal C, and it works. > I think we should call this a trivial patch and just apply it. > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> I agree with your reasoning.
On 10 June 2011 09:14, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On 9 June 2011 18:44, Andreas Färber <andreas.faerber@web.de> wrote: >>> Am 09.06.2011 um 17:52 schrieb Marc-Antoine Perennou: >>>> Manually including stddef.h or replacing NULL by 0 or (void*)0 makes it >>>> work. >>> >>> Then please submit a new patch that explicit includes that header with a >>> comment explaining why, so that it doesn't get removed by accident. >> >> I think the original patch is fine. This configure test isn't >> trying to test whether we correctly found some definition of >> NULL, it's testing pulseaudio presence. The test code should >> be the simplest and most stand-alone code that achieves that. >> There's no need to go dragging in extra headers when we don't >> even need to be using NULL here anyhow. >> >> We already have configure tests which use unadorned 0 for >> pointers (eg the pthread test), it's legal C, and it works. >> I think we should call this a trivial patch and just apply it. >> >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > I agree with your reasoning. > If you want me to provide a patch with the extra include instead, I can, otherwise, could this trivial one get merged so qemu-kvm can be built on several systems on which it actually can't ?
On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote: > pulse/simple.h does not include stdlib.h > We cannot use NULL since it may not be defined > Use 0 instead > > Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com> > --- > configure | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) There has been discussion but I hope we've arrived at consensus based on the fact that 0 is already used for pthreads and it's valid C. Thanks, applied to the trivial patches tree: http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches Stefan
On Fri, Jun 24, 2011 at 2:31 PM, Marc-Antoine Perennou <Marc-Antoine@perennou.com> wrote: > On 10 June 2011 09:14, Markus Armbruster <armbru@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> On 9 June 2011 18:44, Andreas Färber <andreas.faerber@web.de> wrote: >>>> Am 09.06.2011 um 17:52 schrieb Marc-Antoine Perennou: >>>>> Manually including stddef.h or replacing NULL by 0 or (void*)0 makes it >>>>> work. >>>> >>>> Then please submit a new patch that explicit includes that header with a >>>> comment explaining why, so that it doesn't get removed by accident. >>> >>> I think the original patch is fine. This configure test isn't >>> trying to test whether we correctly found some definition of >>> NULL, it's testing pulseaudio presence. The test code should >>> be the simplest and most stand-alone code that achieves that. >>> There's no need to go dragging in extra headers when we don't >>> even need to be using NULL here anyhow. >>> >>> We already have configure tests which use unadorned 0 for >>> pointers (eg the pthread test), it's legal C, and it works. >>> I think we should call this a trivial patch and just apply it. >>> >>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> >> >> I agree with your reasoning. >> > > If you want me to provide a patch with the extra include instead, I > can, otherwise, could this trivial one get merged so qemu-kvm can be > built on several systems on which it actually can't ? It is in the trivial-patches tree now. qemu.git will pick it up next and then qemu-kvm.git will pull it in when it syncs up with qemu.git. Stefan
diff --git a/configure b/configure index ea8b676..d67c3ce 100755 --- a/configure +++ b/configure @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do pa) audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \ - "pa_simple *s = NULL; pa_simple_free(s); return 0;" + "pa_simple *s = 0; pa_simple_free(s); return 0;" libs_softmmu="-lpulse -lpulse-simple $libs_softmmu" audio_pt_int="yes" ;;
pulse/simple.h does not include stdlib.h We cannot use NULL since it may not be defined Use 0 instead Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com> --- configure | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)