After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 654627 - GParamSpec: intern property names
GParamSpec: intern property names
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gobject
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2011-07-14 16:12 UTC by Allison Karlitskaya (desrt)
Modified: 2011-07-26 11:44 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
GParamSpec: intern property names (1.30 KB, patch)
2011-07-14 16:12 UTC, Allison Karlitskaya (desrt) 		committed Details | Review

Description Allison Karlitskaya (desrt) 2011-07-14 16:12:16 UTC 
gparam.c currently contains some rather curious code for the case that
we are installing a property without the G_PARAM_STATIC_NAME flag:

      pspec->name = g_strdup (name);
      canonicalize_key (pspec->name);
      g_intern_string (pspec->name);

This seems quite useless.  It interns the string, but doesn't use the
interned string for the copy stored in the GParamSpec.  We should just
use it.
Comment 1 Allison Karlitskaya (desrt) 2011-07-14 16:12:19 UTC 
Created attachment 191983 [details] [review]
GParamSpec: intern property names

Make it so that the ->name property on all GParamSpec objects is an
interned string.
Comment 2 Allison Karlitskaya (desrt) 2011-07-14 20:43:46 UTC 
We could also consider changing the 'gchar *name' in the GParamSpec struct.  This is theoretically an API break but is only noticed if people are accessing it directly in unsafe ways (ie: via non-const pointer).  Even then, it's just a warning.
Comment 3 Matthias Clasen 2011-07-15 00:17:31 UTC 
Review of attachment 191983 [details] [review]:

Looks fine to me; has the feel of a microoptimization, if anything.

We should, however, document that pspec->name is an interned string, if we want to guarantee this.
Comment 4 Allison Karlitskaya (desrt) 2011-07-15 08:25:40 UTC 
Comment on attachment 191983 [details] [review]
GParamSpec: intern property names

The fact that the string is always interned is the main point of this patch, so addings docs for that is clearly a good idea.  I did that and pushed the commit.

The question remains about if we should change the "gchar *" to "const gchar *".  It's certainly the "more correct" thing to do.
Comment 5 David Zeuthen (not reading bugmail) 2011-07-19 19:58:44 UTC 
I think this change breaks gtk3.0.x with glib master:

(gdb) bt

+ Trace 227814

  • #0 __GI_raise
    at ../nptl/sysdeps/unix/sysv/linux/raise.c line 64
  • #1 __GI_abort
    at abort.c line 93
  • #2 __libc_message
    at ../sysdeps/unix/sysv/linux/libc_fatal.c line 198
  • #3 malloc_printerr
  • #4 standard_free
    at gmem.c line 101
  • #5 g_free
    at gmem.c line 263
  • #6 gtk_theming_engine_register_property
    at gtkthemingengine.c line 384
  • #7 ??
    from /usr/lib64/gtk-3.0/3.0.0/theming-engines/libadwaita.so
  • #8 type_class_init_Wm
    at gtype.c line 2212
  • #9 g_type_class_ref
    at gtype.c line 2912
  • #10 g_object_new_valist
    at gobject.c line 1571
  • #11 g_object_new
    at gobject.c line 1325
  • #12 create_engine
    from /usr/lib64/gtk-3.0/3.0.0/theming-engines/libadwaita.so
  • #13 gtk_theming_engine_load
    at gtkthemingengine.c line 1008
  • #14 css_provider_parse_value
    at gtkcssprovider.c line 2911
  • #15 parse_rule
    at gtkcssprovider.c line 3482
  • #16 parse_stylesheet
    at gtkcssprovider.c line 3542
  • #17 gtk_css_provider_load_from_path_internal
    at gtkcssprovider.c line 3719
  • #18 parse_rule
    at gtkcssprovider.c line 3299
  • #19 parse_stylesheet
    at gtkcssprovider.c line 3542
  • #20 gtk_css_provider_load_from_path_internal
    at gtkcssprovider.c line 3719
  • #21 gtk_css_provider_get_named
    at gtkcssprovider.c line 4236
  • #22 settings_update_theme
    at gtksettings.c line 2739
  • #23 settings_init_style
    at gtksettings.c line 1443
  • #24 gtk_settings_get_for_screen
    at gtksettings.c line 1477
  • #25 display_opened_cb
    at gtkmodules.c line 494
  • #26 g_cclosure_marshal_VOID__OBJECT
    at gmarshal.c line 644
  • #27 g_closure_invoke
    at gclosure.c line 773
  • #28 signal_emit_unlocked_R
    at gsignal.c line 3271
  • #29 g_signal_emit_valist
    at gsignal.c line 3002
  • #30 g_signal_emit_by_name
    at gsignal.c line 3096
  • #31 _gdk_x11_display_open
    at gdkdisplay-x11.c line 1419
  • #32 gdk_x11_display_manager_open_display
    at gdkdisplaymanager-x11.c line 55
  • #33 gtk_init_check
    at gtkmain.c line 1132
  • #34 gtk_init
    at gtkmain.c line 1184
  • #35 gdu_application_new
    at gduapplication.c line 130
  • #36 main
    at main.c line 34 






Comment 6


David Zeuthen (not reading bugmail)





          2011-07-19 20:04:00 UTC
        



<cosimoc> davidz, that's fixed already in gtk master
<davidz> cosimoc: I shouldn't have to install gtk+ master just to use glib master...
 (and I never do since I don't really hack on gtk+ but I hack a lot on glib)
 cosimoc: but of course if it's a gtk+ bug trigger by a legitimate change then, meh
<-- gogo13 has quit (gogo13)
<cosimoc> davidz, GTK+ used an evil hack indeed
<cosimoc> not that the current hack is much better
<davidz> ok






Comment 7


Allison Karlitskaya (desrt)





          2011-07-20 11:19:42 UTC
        



"Officially" I consider that Gtk had a bug.

The fact that the code in question contains /* FIXME: hack hack hack */ just before the problematic code should help convince you of the same. :)






Comment 8


Matthias Clasen





          2011-07-21 04:53:00 UTC
        



Clearly, the gtk side is to blame here. I'll try to get a release out soon to have things work together again.