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 755048 - Stop use of Glib::RefPtr with certain classes
Stop use of Glib::RefPtr with certain classes
Status: RESOLVED FIXED
Product: glibmm
Classification: Bindings
Component: general
2.45.x
Other All
: Normal enhancement
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2015-09-15 09:47 UTC by Kjell Ahlstedt
Modified: 2015-09-16 09:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch: Glib::RefPtr: Enable disallowance with certain classes (1.74 KB, patch)
2015-09-15 09:53 UTC, Kjell Ahlstedt
none Details | Review
Test case (401 bytes, text/plain)
2015-09-15 09:54 UTC, Kjell Ahlstedt
  Details

Description Kjell Ahlstedt 2015-09-15 09:47:44 UTC
Glib::RefPtr is not supposed to be used with Gtk::Widget, although Widget is
a subclass of Glib::Object. It would be fine, if people who try to store a
Widget pointer in a RefPtr get a compile-time error message.
This is possible to achieve with a static_assert in RefPtr and a special
typedef in Widget.

See also bug 583399 comment 6:
> I wish we could also stop RefPtr from being used with Widget classes.

and comments 12 and 16.
Comment 1 Kjell Ahlstedt 2015-09-15 09:53:54 UTC
Created attachment 311341 [details] [review]
patch: Glib::RefPtr: Enable disallowance with certain classes
Comment 2 Kjell Ahlstedt 2015-09-15 09:54:39 UTC
Created attachment 311342 [details]
Test case
Comment 3 Murray Cumming 2015-09-15 09:59:32 UTC
Would there be any way for people to still do this if they really really wanted to? I can imagine someone insisting that it works for them and being annoyed that they have to change their code.
Comment 4 Kjell Ahlstedt 2015-09-15 10:46:48 UTC
(In reply to Murray Cumming from comment #3)
> Would there be any way for people to still do this if they really really
> wanted to?

Not with the present patch, I think.

It can be fixed by surrounding the new code by #ifndef/#endif.

  #ifndef GLIBMM_ALLOW_REFPTR_WITH_ANY_CLASS_ALTHOUGH_IT_IS_UNWISE
    ...
  #endif

It's probably possible to find a better name for the preprocessor constant.
Comment 5 Murray Cumming 2015-09-15 12:10:26 UTC
Maybe we should just surround it with 
  #ifndef GTKMM_DISABLE_DEPRECATED
  #endif

Then they will only be stopped from using it when they don't want to use deprecated API. That seems reasonable.
Comment 6 Kjell Ahlstedt 2015-09-15 17:13:34 UTC
That's a good idea, provided you reverse the test. This is the opposite of
deprecated code.

  #ifdef GTKMM_DISABLE_DEPRECATED
  #endif

GTKMM_DISABLE_DEPRECATED or GLIBMM_DISABLE_DEPRECATED? When I wrote comment 4,
I was thinking of #if[n]def around the added code in Glib::RefPtr. Then use
GLIBMM_DISABLE_DEPRECATED. If your idea is to put it around
  typedef int dont_allow_use_in_glib_refptr_;
in Gtk::Widget or Gtk::Object, then use GTKMM_DISABLE_DEPRECATED.
On second thought I find that a better choice. Then the patch in comment 1
can be used without modification. Perhaps just add a comment that describes
this trick for conditional activation of the static_assert in RefPtr.
Comment 7 Murray Cumming 2015-09-16 09:08:18 UTC
Thanks. I've pushed the glibmm patch and used it in Gtk::Object:
https://git.gnome.org/browse/gtkmm/commit/?id=ae7bf5bb51423c2c1e16397da7e00c4e6c17f34b