GNOME Bugzilla – Bug 751432
C++11: Add move constructors
Last modified: 2015-08-15 13:21:46 UTC
Created attachment 305994 [details] [review] 0001-C-11-_CLASS_BOXEDTYPE-Add-optional-move-constructors.patch This patch adds a check for C++11 move constructor support, and optionally adds a move constructor to boxed types. Please check and criticise. For instance: - Is the test code (in the build/*.m4) file correct. - Is the generated move constructor correct?
Created attachment 305995 [details] [review] 0001-C-11-_CLASS_BOXEDTYPE-Add-optional-move-constructors.patch
You're very close to creating a segmentation fault there. The move constructor has to move member elements to the new object, not copy them. In your case, if you tried to dereference the something member variable on c (granted the compiler doesn't perform move elision on the returned object, -O0 might disable it), it would access freed memory. This is because moving doesn't mean the destructor isn't called, and if the move constructor doesn't set the pointer to nullptr, the destructor still frees the memory (I'm unsure though if you need an additional check in the destructor when the member might be nullptr). Anyway, are you aware of the ax_cxx_compile_stdcxx_11 autoconf macro [1]? That would be an alternative to writing your own macros; unless you want to feature-test individual C++11 features. [1] https://www.gnu.org/software/autoconf-archive/ax_cxx_compile_stdcxx_11.html
Review of attachment 305995 [details] [review]: ::: build/cxx_11.m4 @@ +46,3 @@ + testclass(testclass&& src) + { + something = src.something; I'd set src.something to nullptr here. Otherwise both this and src will delete the same pointer. But this isn't so important, as the code is only compiled, instead of being linked and run. Anyway, I think that a check like "void f(int&& i) {i = 2;}" would be sufficient. ::: tools/m4/class_boxedtype.m4 @@ +139,3 @@ +__CPPNAME__::__CPPNAME__`'(const __CPPNAME__&& other) +: + gobject_(other.gobject_); The other.gobject_ should be set to nullptr in move-constructor's body.
(In reply to Jonas Platte from comment #2) Thanks. So, we just need to set _gobject to null at the end of the move constructor? > Anyway, are you aware of the ax_cxx_compile_stdcxx_11 autoconf macro [1]? > That would be an alternative to writing your own macros; unless you want to > feature-test individual C++11 features. I did try that, but it makes the build use --std=c++11 if the compiler supports that, without giving us (more importantly, distro packagers) any choice about that. Furthermore, I need to check for something glibmm-specific (with a GLIBMM_ prefix) to avoid any confusion with the checks that an app's build might do, because that ifdef in the header is really about how glibmm was built. And in the past we have always checked per feature anyway.
(In reply to Krzesimir Nowak from comment #3) > The other.gobject_ should be set to nullptr in move-constructor's body. I'd use 0 rather than nullptr, to avoid making it a check for nullptr support.
(In reply to Murray Cumming from comment #5) > (In reply to Krzesimir Nowak from comment #3) > > The other.gobject_ should be set to nullptr in move-constructor's body. > > I'd use 0 rather than nullptr, to avoid making it a check for nullptr > support. That's fine, too.
A class with a move constructor should also have a move assignment operator, shouldn't it? (In reply to Jonas Platte from comment #2) > (I'm unsure though if you need an additional check in the destructor when > the member might be nullptr). No, you don't need that when you just apply the delete operator to the pointer. I've seen tons of unnecessary null pointer tests, like if (p) delete p; From Bjarne Stroustrup: "The C++ Programming Language", 4th edition, section 11.2: "Applying delete to the nullptr has no effect." It's true also in C++ before C++11.
(In reply to Murray Cumming from comment #4) > I did try that, but it makes the build use --std=c++11 if the compiler > supports that, without giving us (more importantly, distro packagers) any > choice about that. Furthermore, I need to check for something > glibmm-specific (with a GLIBMM_ prefix) to avoid any confusion with the > checks that an app's build might do, because that ifdef in the header is > really about how glibmm was built. > > And in the past we have always checked per feature anyway. I would be better to have all or nothing C++11 support. Conditional compilation per feature is doomed to fail by it ever increasing complexity.
(In reply to Krzesimir Nowak from comment #6) > (In reply to Murray Cumming from comment #5) > > (In reply to Krzesimir Nowak from comment #3) > > > The other.gobject_ should be set to nullptr in move-constructor's body. > > > > I'd use 0 rather than nullptr, to avoid making it a check for nullptr > > support. > > That's fine, too. See my comment about C++11 all or nothing. nullptr have been one of the first C++11 features available.
Created attachment 306200 [details] [review] 0001-C-11-_CLASS_BOXEDTYPE-Add-optional-move-constructors3.patch OK. I'll apply this one to master soon if there are no objections, and maybe we can find other uses of C++11 to add in the same way, before we actually require C++11 (which I guess we'll do soon too). This one adds the move assignment operator and nulls the other instances data in the move constructor.
(In reply to Hubert Figuiere from comment #8) > I would be better to have all or nothing C++11 support. Conditional > compilation per feature is doomed to fail by it ever increasing complexity. Possibly though I worry that it might be overly optimistic. For now I think I'd like to have a test per feature as we've always had. If/when we do use just one check for C++11 support, we'll need an m4 macro for that. See why comment #4 about how ax_cxx_compile_stdcxx_11 isn't quite what we want.
Also, we have many private copy constructors and assignment operators, not implemented, to enforce that they are not copyable. Maybe we should do the same for the move equivalents? Then, separately, maybe we could use the "= delete" syntax for these in ifdefs, though that would need to detect the app's compiler ability, not the ability of the compiler used to build glibmm.
(In reply to Murray Cumming from comment #10) > #undef GLIBMM_CXX_11_HAS_RVALUE_REFS 1 Why "1"? An undefined preprocessor macro has no value. I applied your patch, configured and built glibmm with CXXFLAGS=-std=c++11, and tried to build an application without -std=c++11. As expected, it failed. One of the compilation errors is /opt/gnome/include/glibmm-2.4/glibmm/valuearray.h:72:32: error: invalid constructor; you probably meant ‘Glib::ValueArray (const Glib::ValueArray&)’ ValueArray(ValueArray&& other); The declaration of the move constructor in the header file is seen by the app, being compiled without C++11 support. I don't know if this is a good thing or a bad thing, but anyway, if glibmm is built with C++11 support, every app must also be built with C++11 support. If that's an unwanted restriction, perhaps change the guard to #if defined(GLIBMM_CXX_11_HAS_RVALUE_REFS) && __cplusplus >= 201103L (In reply to Murray Cumming from comment #12) > Also, we have many private copy constructors and assignment operators, not > implemented, to enforce that they are not copyable. Maybe we should do the > same for the move equivalents? I don't think it's necessary, but it might still be a good idea. It would make it obvious that such a class can't be moved. See Bjarne Stroustrup: "The C++ Programming Language", 4th edition, section 17.6.
(In reply to Kjell Ahlstedt from comment #13) > (In reply to Murray Cumming from comment #10) > > > #undef GLIBMM_CXX_11_HAS_RVALUE_REFS 1 > > Why "1"? An undefined preprocessor macro has no value. Yes, sorry. That's just a typo. I'll fix it here. > I applied your patch, configured and built glibmm with CXXFLAGS=-std=c++11, > and tried to build an application without -std=c++11. As expected, it failed. > One of the compilation errors is > /opt/gnome/include/glibmm-2.4/glibmm/valuearray.h:72:32: error: invalid > constructor; you probably meant ‘Glib::ValueArray (const > Glib::ValueArray&)’ > ValueArray(ValueArray&& other); > > The declaration of the move constructor in the header file is seen by the > app, > being compiled without C++11 support. > > I don't know if this is a good thing or a bad thing, but anyway, if glibmm is > built with C++11 support, every app must also be built with C++11 support. Yes, that's expected and not unreasonable, I think. I wasn't planning on people using this much anyway, but I'm now leaning towards just requiring C++11 completely - see the mailing list thread.
glibmm now requires C++11, so I pushed a version of this without the ifdefs: https://git.gnome.org/browse/glibmm/commit/?id=1b23397544bd031331b3110be1b790659bdceb24 Other C++11-using patches are very welcome.
Now 'make check' fails in gtkmm, because Gio::SrvTarget contains a move constructor. Probably 'make' will fail if gtkmm is configured without --enable-warnings=fatal, because the deprecated class Glib::ValueArray also contains a move constructor. I suppose this is expected. How shall it be fixed? Shall we add AX_CXX_COMPILE_STDCXX_11([noext],[mandatory]) to configure.ac in all modules that depend on glibmm?
Yes, everything that uses glibmm will need to be built with C++11.
I've done that now, adding AX_CXX_COMPILE_STDCXX_11 in pangomm, atkmm, and gtkmm. Other *mm projects will need it too.
I've just added it in gstreamermm project.
Instead of storing ax_cxx_compile_stdcxx_11.m4 in every *mm module, isn't it possible (and better) to store it only in mm-common, and let the mm-common-prepare command copy it?
Sure. Would we need to prefix it to avoid a conflict?
There is yet another possibility: Store ax_cxx_compile_stdcxx_11.m4 in mm-common/macros, and install it together with the mm-*.m4 files when mm-common is installed. That's the most consistent thing to do: Handle all m4 macros in mm-common equally. Prefix? That would be safer, of course. There might be a conflict, if another module used another version of ax_cxx_compile_stdcxx_11.m4. I suppose we could prefix the file name with mm- and the macro names with MM_ or (names that begin with _) _MM.
Sorry, but they sound like the same thing to me. I don't see the difference. In general, yes, the simple choice is the best. Yes, if there can be a conflict then we should avoid the conflict, I guess. Unfortunately that would make it hard to keep it in sync with the original version: http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=history;f=m4/ax_cxx_compile_stdcxx_11.m4
Alternative 1: Store ax_cxx_compile_stdcxx_11.m4 in either mm-common/build or mm-common/macros. Let mm-common-prepare copy it to ./build, like it copies mm-common/build/*.am. The aclocal command finds the file in ./build. Alternative 2: Store ax_cxx_compile_stdcxx_11.m4 in mm-common/macros. Handle it like the other mm-common/macros/*.m4 files, i.e. install it in $(prefix)/share/aclocal when mm-common is installed. The aclocal command finds it there.
Yes, alternative 2 seems simplest. If you add namespace prefixes, please do that in a separate commit. That might make it easier to keep it in sync.
I have pushed 2 commits to mm-common. The second one only adds the prefix. I have tested it on libsigc++. Why don't we use the post-release version incrementing, recommended at https://wiki.gnome.org/MaintainersCorner/Releasing? Then the version of mm-common would be increased from 0.9.7 to 0.9.8 now, and modules that use MM_AX_CXX_COMPILE_STDCXX_11() could MM_PREREQ([0.9.8]), even though mm-common 0.9.8 is not released yet.
Thanks. I've used that in glibmm and it seems fine. I'd rather avoid the complication of post-release version incrementing, please, particularly while GTK+ doesn't do it. We can just a new mm-common release.
(In reply to Murray Cumming from comment #15) > glibmm now requires C++11, so I pushed a version of this without the ifdefs: > https://git.gnome.org/browse/glibmm/commit/ > ?id=1b23397544bd031331b3110be1b790659bdceb24 Instead of this: __CPPNAME__::__CPPNAME__`'(__CPPNAME__&& other) : gobject_(other.gobject_) { other.gobject_ = 0; } should it be this? __CPPNAME__::__CPPNAME__`'(__CPPNAME__&& other) : gobject_(std::move(other.gobject_)) { other.gobject_ = 0; } just as a matter of style and consistency?
(In reply to Murray Cumming from comment #28) > should it be this? > > > __CPPNAME__::__CPPNAME__`'(__CPPNAME__&& other) > : > gobject_(std::move(other.gobject_)) > { > other.gobject_ = 0; > } > > just as a matter of style and consistency? As far as gobject_'s type has no move constructor, gobject_(std::move(other.gobject_)) and gobject_(other.gobject_) should have the same effect - copy constructor of gobject_ is called. Type of gobject_ is (almost?) always pure C structure, so I think we can assume, that it doesn't have move constructor. But... because of "almost?", IMO it's safer to use construction with std::move call.
I just noted, that it's BOXED_TYPE constructor, and gobject_ is a pointer. So, both constructions (with and without std::move) have the same effect in this case.