fetching unset GParamSpec property

[Kevin Ryde]

The program below gets some errors followed by a segv

    GLib-GObject-CRITICAL **: g_param_spec_ref: assertion `G_IS_PARAM_SPEC (pspec)' failed at /home/gg/bug/default-value/param-null.pl line 19.
    GLib-GObject-CRITICAL **: g_param_spec_sink: assertion `G_IS_PARAM_SPEC (pspec)' failed at /home/gg/bug/default-value/param-null.pl line 19.
    GLib-GObject-CRITICAL **: g_param_spec_get_name: assertion `G_IS_PARAM_SPEC (pspec)' failed at /home/gg/bug/default-value/param-null.pl line 19.

I guess newSVGParamSpec() expect NULL.  Is it meant to?  Or is it up to
_gperl_sv_from_value_internal(), per below?

Maybe nobody ever used a paramspec property before. :-)  I can't think
what it'd be for.  An object factory object thing maybe.


--
"The only problem with Italian food is that 4 or 5 days later you start
feeling a bit peckish again."



package MyThing;
use strict;
use warnings;
use Glib;

use Glib::Object::Subclass
  Glib::Object::,
  properties => [ Glib::ParamSpec->param_spec ('myprop', 'myprop',
                                               'Blurb.',
                                               'Glib::Param::Boolean',
                                               Glib::G_PARAM_READWRITE) ];

package main;
use strict;
use warnings;
use Data::Dumper;

my $obj = MyThing->new;
my $x = $obj->get ('myprop');
print Dumper($x);

exit 0;

---

--- GValue.xs 10 Jan 2008 09:50:05 +1100 1.22
+++ GValue.xs 23 Jun 2008 17:49:34 +1000
@@ -244,7 +244,16 @@
                                                  FALSE);
 
  case G_TYPE_PARAM:
- return newSVGParamSpec (g_value_get_param (value));
+ /* can have NULL here fetching object properties of
+   type G_TYPE_PARAM with no value set yet, or from
+   ->get_default_value of such a property */
+ {
+ GParamSpec *ps = g_value_get_param (value);
+ if (ps == NULL)
+ return &PL_sv_undef;
+ else
+ return newSVGParamSpec (ps);
+ }
 
  case G_TYPE_OBJECT:
  return gperl_new_object (g_value_get_object (value), FALSE);

---

_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Torsten Schoenfeld]

Kevin Ryde wrote:
> The program below gets some errors followed by a segv
>
>     GLib-GObject-CRITICAL **: g_param_spec_ref: assertion `G_IS_PARAM_SPEC (pspec)' failed at /home/gg/bug/default-value/param-null.pl line 19.
>     GLib-GObject-CRITICAL **: g_param_spec_sink: assertion `G_IS_PARAM_SPEC (pspec)' failed at /home/gg/bug/default-value/param-null.pl line 19.
>     GLib-GObject-CRITICAL **: g_param_spec_get_name: assertion `G_IS_PARAM_SPEC (pspec)' failed at /home/gg/bug/default-value/param-null.pl line 19.
>
> I guess newSVGParamSpec() expect NULL.  Is it meant to?  Or is it up to
> _gperl_sv_from_value_internal(), per below?

I think newSVGParamSpec should follow the precedent of gperl_new_object,
gperl_new_boxed, and newSVGChar -- accept NULL and return undef.  As in the
attached patch.

muppet?

--
Bye,
-Torsten

Index: GParamSpec.xs
===================================================================
RCS file: /cvsroot/gtk2-perl/gtk2-perl-xs/Glib/GParamSpec.xs,v
retrieving revision 1.23
diff -u -d -p -r1.23 GParamSpec.xs
--- GParamSpec.xs 17 Oct 2005 19:26:09 -0000 1.23
+++ GParamSpec.xs 13 Jul 2008 15:34:57 -0000
@@ -147,6 +147,9 @@ newSVGParamSpec (GParamSpec * pspec)
  HV * stash;
  const char * package;
 
+ if (!pspec)
+ return &PL_sv_undef;
+
  g_param_spec_ref (pspec);
  g_param_spec_sink (pspec);
 
Index: t/e.t
===================================================================
RCS file: /cvsroot/gtk2-perl/gtk2-perl-xs/Glib/t/e.t,v
retrieving revision 1.2
diff -u -d -p -r1.2 e.t
--- t/e.t 20 Oct 2004 17:33:56 -0000 1.2
+++ t/e.t 13 Jul 2008 15:34:57 -0000
@@ -1,9 +1,10 @@
+#!/usr/bin/perl
 #
 # ParamSpec stuff.
 #
 use strict;
 use Glib ':constants';
-use Test::More tests => 231;
+use Test::More tests => 232;
 
 # first register some types with which to play below.
 
@@ -204,3 +205,13 @@ Glib::Type->register (
 foreach (@params) {
  is ($_->get_owner_type, 'Bar', ref($_)." owner type after adding");
 }
+
+
+
+#
+# verify that NULL param specs are handled gracefully
+#
+
+my $object = Bar->new;
+my $x = $object->get ('param_spec');
+is ($x, undef);

_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[muppet]

On Jul 13, 2008, at 11:35 AM, Torsten Schoenfeld wrote:

> Kevin Ryde wrote:
>> The program below gets some errors followed by a segv
>>
>>    GLib-GObject-CRITICAL **: g_param_spec_ref: assertion  
>> `G_IS_PARAM_SPEC (pspec)' failed at /home/gg/bug/default-value/
>> param-null.pl line 19.
>>    GLib-GObject-CRITICAL **: g_param_spec_sink: assertion  
>> `G_IS_PARAM_SPEC (pspec)' failed at /home/gg/bug/default-value/
>> param-null.pl line 19.
>>    GLib-GObject-CRITICAL **: g_param_spec_get_name: assertion  
>> `G_IS_PARAM_SPEC (pspec)' failed at /home/gg/bug/default-value/
>> param-null.pl line 19.
>>
>> I guess newSVGParamSpec() expect NULL.  Is it meant to?  Or is it  
>> up to
>> _gperl_sv_from_value_internal(), per below?
>
> I think newSVGParamSpec should follow the precedent of  
> gperl_new_object,
> gperl_new_boxed, and newSVGChar -- accept NULL and return undef.  As  
> in the
> attached patch.
>
> muppet?

I started to write "but that would render newSVGParamSpec_ornull()  
obsolete", but then i looked at gperl.h and there is no such symbol.

The original intent was for the newSVType() function to verify you  
actually had the type, and newSVType_ornull() to allow NULL, so that  
you didn't accidentally allow NULL everywhere and create warnings and  
assertions from the C code where you could have caught that with  
better type information in the bindings.  With that in mind, using an  
_ornull in gperl_sv_from_value() makes sense.  Well, actually, it  
kinda sucks, but that's our best hook into the property machinery, so  
that's how it happens.

But, as you point out, there are some heavy hitters that disobey this  
rule.  I'm at a loss for why.  :-(


--
Leia/Lois:  Aren't you a little fat for a stormtrooper?
Luke/Chris: Well, stay here and rot, you stuck-up bitch.
   -- Family Guy, "Blue Harvest" (A "Star Wars" parody)

_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Kevin Ryde]

muppet <scott@...> writes:
>
> I started to write "but that would render newSVGParamSpec_ornull()
> obsolete", but then i looked at gperl.h and there is no such symbol.

Speaking of ornull, I struck another NULL, this time through a mistake.
It looks like g_param_spec_int() and friends return NULL when the
default is outside the min/max, so the following gets a segv.

    use strict;
    use warnings;
    use Gtk2;
    Glib::ParamSpec->int ('foo',
                          'foo',
                          'Blurb.',
                          -1, 0, 999,
                          Glib::G_PARAM_READWRITE);

It prints g_log warnings, then goes bang.  I guess there'd be a choice
between returning undef or croaking.  undef would be more like the C and
friendlier for the unlikely case of creating something dynamically.  Or
croaking would be a more usual way for a constructor to fail, and any
typical subclassing isn't going to get much further ...

--
The sigfile one-line movie reviews series:
"2001: A Space Odyssey" -- a documentary about apes.
_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Torsten Schoenfeld]

muppet wrote:
> With that in mind, using an _ornull in gperl_sv_from_value() makes sense.
> Well, actually, it kinda sucks, but that's our best hook into the property
> machinery, so that's how it happens.
>
> But, as you point out, there are some heavy hitters that disobey this rule.
> I'm at a loss for why.  :-(

So, what do we do?  Do we make newSVGParamSpec return undef for NULL?  Or do
we make _gperl_sv_from_value_internal handle NULL param specs on its own (and
make newSVGParamSpec croak on NULL for safety)?

--
Bye,
-Torsten
_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Torsten Schoenfeld]

Torsten Schoenfeld wrote:

> So, what do we do?  Do we make newSVGParamSpec return undef for NULL?  Or do
> we make _gperl_sv_from_value_internal handle NULL param specs on its own (and
> make newSVGParamSpec croak on NULL for safety)?

I just committed a patch that implements the latter approach.

--
Bye,
-Torsten
_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Kevin Ryde]

Torsten Schoenfeld <kaffeetisch@...> writes:
>
> I just committed a patch that implements the latter approach.

Don't forget the constructors returning null for bad args too.

---

--- e.t 19 Aug 2008 09:04:49 +1000 1.3
+++ e.t 19 Aug 2008 09:40:31 +1000
@@ -83,6 +83,11 @@
  is ($pspec->get_maximum, $max, "$nick max");
  is ($pspec->get_default_value, $default, "$nick default");
  push @params, $pspec;
+
+        # min/max the wrong way around
+ is (Glib::ParamSpec->$name ($name, $nick, $blurb, $max, $min,
+                                    $default, $flags), undef,
+            "$nick undef from min > max args");
 }
 
 #
@@ -103,6 +108,11 @@
  is_float ($pspec->get_default_value, $default, "$nick default");
  ok ($pspec->get_epsilon > 0.0, "$nick epsilon");
  push @params, $pspec;
+
+        # min/max the wrong way around
+ is (Glib::ParamSpec->$name ($name, $nick, $blurb, $max, $min,
+                                    $default, $flags), undef,
+            "$nick undef from min > max args");
 }
 
 
@@ -175,6 +185,10 @@
 isa_ok ($pspec, 'Glib::Param::Long', 'IV is actually Long');
 push @params, $pspec;
 
+is (Glib::ParamSpec->IV ('iv', 'IV', 'Blurb.', 10, -20, -5, G_PARAM_READWRITE),
+    undef,
+    "IV undef from min > max args");
+
 
 $pspec = Glib::ParamSpec->UV ('uv', 'UV',
                       'This is the same as UInt',
@@ -182,6 +196,10 @@
 isa_ok ($pspec, 'Glib::Param::ULong', 'UV is actually ULong');
 push @params, $pspec;
 
+is (Glib::ParamSpec->UV ('uv', 'uV', 'Blurb.', 20, 10, 15, G_PARAM_READWRITE),
+    undef,
+    "IV undef from min > max args");
+
 
 $pspec = Glib::ParamSpec->scalar ('scalar', 'Scalar',
                           'This is the same as Boxed',

---

--
"And the sign says long-haired freaky people need not apply."

_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Torsten Schoenfeld]

muppet wrote:

>> I think newSVGParamSpec should follow the precedent of gperl_new_object,
>> gperl_new_boxed, and newSVGChar -- accept NULL and return undef.  As
>> in the
>> attached patch.
>
> I started to write "but that would render newSVGParamSpec_ornull()
> obsolete", but then i looked at gperl.h and there is no such symbol.
>
> The original intent was for the newSVType() function to verify you
> actually had the type, and newSVType_ornull() to allow NULL, so that you
> didn't accidentally allow NULL everywhere and create warnings and
> assertions from the C code where you could have caught that with better
> type information in the bindings.  With that in mind, using an _ornull
> in gperl_sv_from_value() makes sense.  Well, actually, it kinda sucks,
> but that's our best hook into the property machinery, so that's how it
> happens.

Looking at Kevin's recent test patch prompted me to think about this again.  I
now think that what you write above applies to SvFoo converters, but not to
newSVFoo.  SvFoo does SV*->Foo* in which case it is important that the result is
not NULL unless an _ornull variant was used, for the reasons you describe.  But
newSVFoo does Foo*->SV* and returning undef for NULL here is not a big problem.
 In fact, I'm unable to find any Foo*->SV* converter in Glib that chokes on
NULL.  Most simply return undef.

So I think newSVGParamSpec should do the same: return undef for NULL.  In that
case the change to gperl_sv_from_value can be reverted, and the various
Glib::ParamSpec constructors will also automatically start returning undef for
invalid input.

--
Bye,
-Torsten
_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Torsten Schoenfeld]

Torsten Schoenfeld wrote:

> Looking at Kevin's recent test patch prompted me to think about this again.  I
> now think that what you write above applies to SvFoo converters, but not to
> newSVFoo.  SvFoo does SV*->Foo* in which case it is important that the result is
> not NULL unless an _ornull variant was used, for the reasons you describe.  But
> newSVFoo does Foo*->SV* and returning undef for NULL here is not a big problem.
>  In fact, I'm unable to find any Foo*->SV* converter in Glib that chokes on
> NULL.  Most simply return undef.
>
> So I think newSVGParamSpec should do the same: return undef for NULL.  In that
> case the change to gperl_sv_from_value can be reverted, and the various
> Glib::ParamSpec constructors will also automatically start returning undef for
> invalid input.

I just committed this change, for the reasons described above.  newSVGParamSpec
now returns undef for NULL, and gperl_sv_from_value doesn't special-case NULL
GParamSpecs.

-Torsten
_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Torsten Schoenfeld]

Kevin Ryde wrote:
>> I just committed a patch that implements the latter approach.
>
> Don't forget the constructors returning null for bad args too.

With the recent commit, these tests now actually pass.  Unfortunately, they also
print very noisy warning messages, so unless there's a way to suppress those, I
don't think we should add the tests.  Handling NULL GParamSpecs is tested
already at the end of t/e.t anyway.

-Torsten
_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[muppet]

On Sep 6, 2008, at 9:32 AM, Torsten Schoenfeld wrote:

> Kevin Ryde wrote:
>>> I just committed a patch that implements the latter approach.
>>
>> Don't forget the constructors returning null for bad args too.
>
> With the recent commit, these tests now actually pass.  
> Unfortunately, they also
> print very noisy warning messages, so unless there's a way to  
> suppress those, I
> don't think we should add the tests.  Handling NULL GParamSpecs is  
> tested
> already at the end of t/e.t anyway.

You *can* suppress those messages, by using a log handler.

     Glib::Log->set_handler ('GLib-GObject', 'critical', sub {
             print "Hijacked failed assertion message: $_[2]\n";
     });

I'd be tempted to add an ok(1) in there, but you can build glib with  
assertions turned off.

--
Applause is an addiction, like heroin or checking your email.
   -- Sideshow Mel

_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list

[Kevin Ryde]

muppet <scott@...> writes:
>
>     Glib::Log->set_handler ('GLib-GObject', 'critical', sub {
>             print "Hijacked failed assertion message: $_[2]\n";
>     });

You'd be tempted to make any logged messages fatal in the test suite,
perhaps with selected exceptions that are meant to exercise dodgy stuff.

> I'd be tempted to add an ok(1) in there, but you can build glib with
> assertions turned off.

Ah, so the g_return_val_if_fail() in g_param_spec_int() for min>max can
be completely omitted ... which means the return is not necessarily
undef in the tests I wrote anyway.
_______________________________________________
gtk-perl-list mailing list
gtk-perl-list@...
http://mail.gnome.org/mailman/listinfo/gtk-perl-list