GNOME Bugzilla – Bug 761665
Replace Gtk::manage() with std::make_unique<>() and std::move() ?
Last modified: 2018-05-22 12:14:34 UTC
Created attachment 320569 [details] [review] 0001-Trying-to-replace-Gtk-manage-with-std-make_unique-an.patch 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 comments. There are also still some missing method overloads, marked by TODO comments. This was inspired by parts of Herb Sutter's "Back to the Basics: Essentials of Modern C++ Style" talk: https://www.youtube.com/watch?v=xnqTKD8uD64 (slides: https://github.com/CppCon/CppCon2014/blob/master/Presentations/Back%20to%20the%20Basics!%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 )
Created attachment 320674 [details] [review] 0001-Trying-to-replace-Gtk-manage-with-std-make_unique-an2.patch 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: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#i11-never-transfer-ownership-by-a-raw-pointer-t https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-unique_ptr https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#f7-for-general-use-take-t-or-t-arguments-rather-than-smart-pointers
I filed this bug about those guidelines: https://github.com/isocpp/CppCoreGuidelines/issues/522
Now that this part of the guidelines has been improved: https://github.com/isocpp/CppCoreGuidelines/issues/514 I feel better about this patch. I am tempted to apply it. Any objections?
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"); button_box->add(std::move(button)); it would be possible to simplify further to button_box->add(std::make_unique<Gtk::Button>("_OK")); 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.
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.
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.
-- 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: https://gitlab.gnome.org/GNOME/gtkmm/issues/11.