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 761665 - Replace Gtk::manage() with std::make_unique<>() and std::move() ?
Replace Gtk::manage() with std::make_unique<>() and std::move() ?
Product: gtkmm
Classification: Bindings
Component: general
Other Linux
: Normal normal
: ---
Assigned To: gtkmm-forge
Depends on:
Reported: 2016-02-07 11:52 UTC by Murray Cumming
Modified: 2018-05-22 12:14 UTC
See Also:
GNOME target: ---
GNOME version: ---

0001-Trying-to-replace-Gtk-manage-with-std-make_unique-an.patch (61.88 KB, patch)
2016-02-07 11:52 UTC, Murray Cumming
none Details | Review
0001-Trying-to-replace-Gtk-manage-with-std-make_unique-an2.patch (62.56 KB, patch)
2016-02-08 23:08 UTC, Murray Cumming
none Details | Review

Description Murray Cumming 2016-02-07 11:52:46 UTC
Created attachment 320569 [details] [review]

This patch tries to replace Gtk::manage() with std::make_unique() and std::move().
    * gtk/src/*.[hg|ccg]: Add overloads for add(), append(), attach(), etc,
      methods, taking a std::unique_ptr<> by value.
    * demos/tests: Change uses of Gtk::manage() followed by add(*thing)
      to std::make_unique<>() followed by add(std::move(thing)).
      This required some lines of code to be rearranged to avoid us using
      something after it has been std::move()ed. Typically the
      add(std::move(thing)) lines end up all being at the end of the method.
      Some API currently makes it impossible to do this properly.
      See the
      //TODO: Use after move
      There are also still some missing method overloads, marked by TODO
    This was inspired by parts of Herb Sutter's
    "Back to the Basics: Essentials of Modern C++ Style" talk:
    (slides:!%20Essentials%20of%20Modern%20C%2B%2B%20Style/Back%20to%20the%20Basics!%20Essentials%20of%20Modern%20C%2B%2B%20Style%20-%20Herb%20Sutter%20-%20CppCon%202014.pdf )
Comment 1 Murray Cumming 2016-02-08 23:08:33 UTC
Created attachment 320674 [details] [review]

This improved patch deals with those "TODO: use after move" issues by taking an "observing" (not "owning") pointer just before the std::move(). I don't like it, but it's maybe what's expected with modern C++.

For instance, though none of these have a real example of a correct use of unique_ptr<> to indicate passing of ownership:
Comment 2 Murray Cumming 2016-02-08 23:22:20 UTC
I filed this bug about those guidelines:
Comment 3 Murray Cumming 2016-02-26 09:09:59 UTC
Now that this part of the guidelines has been improved:
I feel better about this patch.

I am tempted to apply it. Any objections?
Comment 4 Kjell Ahlstedt 2016-02-28 19:22:52 UTC
My comments after a fairly quick review. I haven't tested the code.

A. In simple cases, such as

  auto button = std::make_unique<Gtk::Button>("_OK");

it would be possible to simplify further to


B. The modified description of Gtk::manage() is a bit strange. It becomes

  /** Mark a Gtk::Object as owned by its parent container widget, so you don't need to delete it manually.
   * For instance,
   * @code
   * auto button = std::make_unique<Gtk::Button>("Hello");
   * vbox.pack_start(*button); //vbox will delete button when vbox is deleted.
   * @endcode
   * @param obj A Gtk::Object, such as a gtkmm widget.
   * @result The Gtk::Object passed as the @a obj parameter.

The code snippet won't compile. And it's odd that the description of manage()
only shows an example that does not use manage().

C. What will happen when we require C++14? Will it upset people almost as much
as when we required C++11?

Apart from comment B, I have no objections.
Comment 5 Murray Cumming 2016-02-29 13:37:47 UTC
That documentation change must just be a mistake. I'll change that back.

We only need C++14 for std::make_unique<> in the examples. So we could commit the add(unique_ptr<>) overloads without depending on C++14. And we could consider using it in the examples when we depend on C++14, around the time that gcc 6 is widespread (with C++14 by default), for instance.
Comment 6 Murray Cumming 2016-03-11 10:36:14 UTC
I plan to commit this, without deprecating Gtk::manage(), but probably not for gtkmm 3.20 because there would not be enough time for people to test it.
Comment 7 GNOME Infrastructure Team 2018-05-22 12:14:34 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: