Hi, On Tue, Jan 22, 2019 at 10:19:59AM -0500, Frediano Ziglio wrote: > > > > Hi, > > > > On Tue, Jan 22, 2019 at 08:01:33AM -0500, Frediano Ziglio wrote: > > > > > > > > From: Victor Toso <[email protected]> > > > > > > > > Can't happen as GstElement *e is not NULL but doing extra G_OBJECT(e) > > > > cast > > > > guarantees this for compiler. > > > > > > > > > spice-gstaudio.c: In function ‘playback_volume_changed’: > > > > > /usr/include/glib-2.0/gobject/gtype.h:2280:70: error: potential null > > > > > pointer dereference [-Werror=null-dereference] > > > > > #define _G_TYPE_IGC(ip, gt, ct) ((ct*) (((GTypeInstance*) > > > > > ip)->g_class)) > > > > > > > > > ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~ > > > > > /usr/include/glib-2.0/gobject/gtype.h:525:66: note: in expansion of > > > > > macro > > > > > ‘_G_TYPE_IGC’ > > > > > #define G_TYPE_INSTANCE_GET_CLASS(instance, g_type, c_type) > > > > > (_G_TYPE_IGC ((instance), (g_type), c_type)) > > > > > > > > > ^~~~~~~~~~~ > > > > > /usr/include/glib-2.0/gobject/gobject.h:86:38: note: in expansion of > > > > > macro > > > > > ‘G_TYPE_INSTANCE_GET_CLASS’ > > > > > #define G_OBJECT_GET_CLASS(object) (G_TYPE_INSTANCE_GET_CLASS > > > > > ((object), > > > > > G_TYPE_OBJECT, GObjectClass)) > > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > spice-gstaudio.c:373:45: note: in expansion of macro > > > > > ‘G_OBJECT_GET_CLASS’ > > > > > } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e), > > > > > "volume") != NULL) { > > > > ^~~~~~~~~~~~~~~~~~ > > > > > > > > Signed-off-by: Victor Toso <[email protected]> > > > > > > Why not adding g_return_if_fail(e != NULL) before using "e" instead ? > > > > Why that would be better than a proper G_OBJECT()? > > > > It looks less weird and checking for NULL costs less.
I think it looks weirder 0:-) just because it should not be null
as there was a check already; but I agree that it should cost
less.
> G_OBJECT is a bit weird. GObject cast macros check for NULL and
> if the type if the type specified assuming is a GObject
> pointer, however in this case (beside the NULL check) is
> checking that is a GObject pointer assuming is a GObject
> pointer so is a tautology. For the compiler is just a trick to
> make it happy.
Yes
> The g_return_if_fail is cheaper as the G_OBJECT macro will call
> a function in any case while g_return_if_fail will call a
> function (g_warning) only if "e" is NULL.
Waiting for pipeline to run
https://gitlab.freedesktop.org/victortoso/spice-gtk/pipelines/15953
>
> > We already check if playback.sink or record.src are null early on
> > in those functions, adding g_return_if_fail() would only check if
> > the assignment was done. I don't think it is needed.
> >
> > Cheers,
> >
> > > > ---
> > > >
> > > > Working CI:
> > > > https://gitlab.freedesktop.org/victortoso/spice-gtk/pipelines/15885
> > > >
> > > > src/spice-gstaudio.c | 8 ++++----
> > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > > > index 51ff028..b2d9cd3 100644
> > > > --- a/src/spice-gstaudio.c
> > > > +++ b/src/spice-gstaudio.c
> > > > @@ -370,7 +370,7 @@ static void playback_volume_changed(GObject *object,
> > > > GParamSpec *pspec, gpointer
> > > >
> > > > if (GST_IS_STREAM_VOLUME(e)) {
> > > > gst_stream_volume_set_volume(GST_STREAM_VOLUME(e),
> > > > GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> > > > - } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> > > > "volume") != NULL) {
> > > > + } else if
> > > > (g_object_class_find_property(G_OBJECT_GET_CLASS(G_OBJECT(e)),
> > > > "volume") != NULL) {
> > > > g_object_set(e, "volume", vol, NULL);
> > > > } else {
> > > > g_warning("playback: ignoring volume change on %s",
> > > > gst_element_get_name(e));
> > > > @@ -399,7 +399,7 @@ static void playback_mute_changed(GObject *object,
> > > > GParamSpec *pspec, gpointer d
> > > >
> > > > if (GST_IS_STREAM_VOLUME(e)) {
> > > > gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> > > > - } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> > > > "mute")
> > > > != NULL) {
> > > > + } else if
> > > > (g_object_class_find_property(G_OBJECT_GET_CLASS(G_OBJECT(e)),
> > > > "mute") != NULL) {
> > > > g_object_set(e, "mute", mute, NULL);
> > > > } else {
> > > > g_warning("playback: ignoring mute change on %s",
> > > > gst_element_get_name(e));
> > > > @@ -437,7 +437,7 @@ static void record_volume_changed(GObject *object,
> > > > GParamSpec *pspec, gpointer d
> > > >
> > > > if (GST_IS_STREAM_VOLUME(e)) {
> > > > gst_stream_volume_set_volume(GST_STREAM_VOLUME(e),
> > > > GST_STREAM_VOLUME_FORMAT_CUBIC, vol);
> > > > - } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> > > > "volume") != NULL) {
> > > > + } else if
> > > > (g_object_class_find_property(G_OBJECT_GET_CLASS(G_OBJECT(e)),
> > > > "volume") != NULL) {
> > > > g_object_set(e, "volume", vol, NULL);
> > > > } else {
> > > > g_warning("record: ignoring volume change on %s",
> > > > gst_element_get_name(e));
> > > > @@ -466,7 +466,7 @@ static void record_mute_changed(GObject *object,
> > > > GParamSpec *pspec, gpointer dat
> > > >
> > > > if (GST_IS_STREAM_VOLUME (e)) {
> > > > gst_stream_volume_set_mute(GST_STREAM_VOLUME(e), mute);
> > > > - } else if (g_object_class_find_property(G_OBJECT_GET_CLASS (e),
> > > > "mute")
> > > > != NULL) {
> > > > + } else if
> > > > (g_object_class_find_property(G_OBJECT_GET_CLASS(G_OBJECT(e)),
> > > > "mute") != NULL) {
> > > > g_object_set(e, "mute", mute, NULL);
> > > > } else {
> > > > g_warning("record: ignoring mute change on %s",
> > > > gst_element_get_name(e));
> > >
> > > Frediano
> >
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/spice-devel
