pencil
pencil copied to clipboard
pencil2d
Improve exception safety with smart pointers
Would you like to wrap any pointer data members with the class template “std::unique_ptr”?
Update candidates:
@elfring Thank you for the suggestions! I'll label this request appropriately so the devs can take a look at it later :smile:
As far as I understand, those all inherit from QObject and are thus properly deleted through Qt own memory management.
Basically any GUI related pointer should be deleted properly when the parent qobject (eg. widget) is deleted, so I'm quite certain that the content in those destructors are redundant.
Yes in typical old c++ you would need to call new and delete later to remove the pointer again, but Qt has its own memory management system that makes sure that when the parent QObject is deleted, all children are deleted too.
https://www.cleanqt.io/blog/crash-course-in-qt-for-c%2B%2B-developers,-part-4
Have you encountered any memory leaking?
Another information source can eventually explain the memory management aspect better. I hope that exception safety can be achieved also for this software by expressing a clear ownership for the involved objects.
@elfring I kinda get what you are saying about memory management. can you explain some more about the exception safety?
And as CandayFace said, Qt has its own memory management strategy. The ownership is specified by setting a parent object. A parent object owns child objects.
For example, in the destructor of RecentFileMenu. It's not necessary to delete these action objects mClearSeparator, mClearAction and mEmptyAction because their parent object is RecentFileMenu. They will be deleted automatically when the RecentFileMenu object is deleted.
If we use the unique_ptr or shared_ptr here, there will be two memory management methods conflicting with each other, which is not good.
It's not necessary to delete these action objects
mClearSeparator,mClearActionandmEmptyActionbecause their parent object is RecentFileMenu.
I suggest to take another look at these implementation details (in which use cases a “parent” was actually specified as a construction parameter).
If we use the unique_ptr or shared_ptr here, there will be two memory management methods conflicting with each other, which is not good.
How do you think about to reduce any explicit calls of the C++ new and delete operators?
It's not necessary to delete these action objects
mClearSeparator,mClearActionandmEmptyActionbecause their parent object is RecentFileMenu.I suggest to take another look at these implementation details (in which use cases a “parent” was actually specified as a construction parameter).
LOL thanks for reminding us. There is a missing "parent" parameter, I will add it.
If we use the unique_ptr or shared_ptr here, there will be two memory management methods conflicting with each other, which is not good.
How do you think about to reduce any explicit calls of the C++ new and delete operators?
Think about the reason behind this rule. We do follow this rule but just in a different way.
For example "The pointer returned by new should belong to a resource handle (that can call delete)". In Pencil2D or any Qt application, a resource handle is the parent object. And we never have to worry about the question like "how can you be certain that you don’t need N+1 or N-1 delete?".
In Pencil2D or any Qt application, a resource handle is the parent object.
How does this information fit to a statement like “uiOptionsBox = new Ui::ImportImageSeqOptions;”?
In Pencil2D or any Qt application, a resource handle is the parent object.
How does this information fit to a statement like “
uiOptionsBox = new Ui::ImportImageSeqOptions;”?
Good question!
Ui::XXX are special classes created by the Qt visual designing tool. They always belong to one specific Qt widget and will be used only in that widget. Their life cycle is identical to the widget. So they are always created in the constructor and deleted in the destructor. They are the "very certain that you just need N delete" case.
But yes, you can use std::unique_str for UI::XXX objects here.
Would you eventually prefer to use another parent parameter for these objects instead?
If we can I would love to, but we can't. Those UI::XXX files are auto-generated by a Qt's codegen tool called uic. We won't be able to change the constructor parameters unless we change the source code of uic.
Sometimes the memory management strategy is not a thing we can choose. We have to follow the Qt way when working with Qt, there is no way to opt out. But we use STL smart pointers when the object is a plain C++ class which doesn't inherit from QObject.
If we can I would love to, but we can't.
Will another look at implementation details become helpful?
Those
UI::XXXfiles are auto-generated by a Qt's codegen tool calleduic.
Does this code generation depend on specific UI files? (I do not see such a file so far which would belong to the class “ImportImageSeqDialog” directly.)
Sometimes the memory management strategy is not a thing we can choose.
I got an other impression for the discussed classes.
@elfring I appreciate your concerns about memory management, and I'm sure there are some things getting leaked here and there from Pencil2D, however I do not think you have brought up anything significant yet. Keep reading...
I agree with what @CandyFace and @chchwy said in the beginning, Qt manages all QObject for us using it's ownership model. When a QObject is deleted, all of it's children (and grandchildren and so on recursively) are deleted. All QObject-based types should have a parent argument in their constructor. When that is specified, the object becomes the child of the argument, and the argument becomes the parent of the child. See the official Qt docs on the topic here: https://doc.qt.io/qt-5/objecttrees.html.
When it comes to generated UI classes, they are not QObjects so they can't be added to the object tree. However, all of the elements within the UI (the widgets that make up the described interface) are QObject so these can have parents when they are created. These are not created in the constructor of the UI class, but rather the setupUI function, which is why that is what takes a parent object as an argument. All of the root elements generated for the interface have their parent set to what you pass with setupUI(). So yes the UI class might get leaked if we fail to delete it correctly, but all that it contains are pointers to widgets which will be deleted properly always because they are managed by Qt. In the end you are talking about an inconsequential number of bytes leaked. For UI::ImportImageSeqDialog, failing to delete it would leak 3 pointers * 8 bytes = ~24 bytes + class overhead.
Having said all this, I definitely wouldn't mind changing uiOptionsBox, uiGroupBoxPreview, and other such UI member variables from regular pointers to unique_ptrs so that we do not have to delete them in the destructor. This will delete the UI files, but will not delete any of the widgets contained within because those aren't deleted in the UI destructor and are instead deleted by Qt when their parent is deleted.
…, however I do not think you have brought up anything significant yet.
:crystal_ball: Some C++ programmers can find the reduction of explicit delete operator calls helpful, can't they?
Some C++ programmers can find the reduction of explicit delete operator calls helpful, can't they?
Yes, that's why I said I was in favour of changing the member variable types.
Those
UI::XXXfiles are auto-generated by a Qt's codegen tool calleduic.Does this code generation depend on specific UI files? (I do not see such a file so far which would belong to the class “ImportImageSeqDialog” directly.)
Ui::ImportImageSeqPreviewGroupBox comes from importimageseqpreview.ui
Ui::ImportImageSeqOptions comes from importimageseqoptions.ui
Is this what you are asking?
Is this what you are asking?
- Partly, yes. I got the impression that another UI file is not used for the mentioned dialogue (while UI files are provided for corresponding widgets).
- Who is going to adjust the discussed implementation details?
I got the impression that another UI file is not used for the mentioned dialogue (while UI files are provided for corresponding widgets).
which ui file is it?
Who is going to adjust the discussed implementation details?
send a pull request whenever you like.