KSVG: fix to include classes used in TQPtrList #41

Merged
MicheleC merged 1 commits from feat/fix-ksvg-canvas_item-delete into master 4 months ago
obache commented 4 months ago
Collaborator

It must be included, or its destructor may not be used in PtrList maitainance.

It must be included, or its destructor may not be used in PtrList maitainance.
obache added 1 commit 4 months ago
fb36eda6aa KSVG: fix to include classes used in TQPtrList
Owner

Hi Obata-san,
I looked at the commit but I am not sure of what the problem is.
TQPtrList is a template list of pointers, so the specified template parameter is only used as a type * in the class. Therefore forward declaration of CanvasItem does not seem to be a problem.
The m_items list also has the autodelete property set to true, so all the objects own by it will be properly destroyed when m_items is destroyed.

Hi Obata-san, I looked at the commit but I am not sure of what the problem is. TQPtrList is a template list of pointers, so the specified template parameter is only used as a ```type *``` in the class. Therefore forward declaration of CanvasItem does not seem to be a problem. The m_items list also has the autodelete property set to true, so all the objects own by it will be properly destroyed when m_items is destroyed.
Owner

I am actually more surprised that CanvasItemList is only forward declared and the compiler does not complain in the declaration of the collisions() method

I am actually more surprised that CanvasItemList is only forward declared and the compiler does not complain in the declaration of the ```collisions()``` method
Poster
Collaborator

Possible problem point is in TQPtrList<type>::deleteItem()

https://mirror.git.trinitydesktop.org/gitea/TDE/tqt3/src/branch/master/src/tools/ntqptrlist.h#L154

delete (type *)d i.e. delete (CanvasItem *)d will probably call CanvasItem::~CanvasItem() and free the memory, with just forward declaration of CanvasItem.

But if destructor of type is virtual function, destructor should be picked up from its virtual table, or if subclass object of type is stored, its destructor will not be called.
And if overload operator delete of type is defined, it must be called istead.

For that to be safe, type must be fully declared.
At least, destructor of CanvasItem is virtual function.

Possible problem point is in `TQPtrList<type>::deleteItem()` https://mirror.git.trinitydesktop.org/gitea/TDE/tqt3/src/branch/master/src/tools/ntqptrlist.h#L154 `delete (type *)d` i.e. `delete (CanvasItem *)d` will probably call `CanvasItem::~CanvasItem()` and free the memory, with just forward declaration of CanvasItem. But if destructor of `type` is virtual function, destructor should be picked up from its virtual table, or if subclass object of `type` is stored, its destructor will not be called. And if overload operator `delete` of `type` is defined, it must be called istead. For that to be safe, `type` must be fully declared. At least, destructor of CanvasItem is virtual function.
Owner

We can merge the PR since it is not a big deal, but I don't think there is a problem in first place. I don't know how the compiler internally instantiate the use of a template, but when it calls the destructor of an object it would call a specific method and the class will have to be well defined at that moment or there would be a compile error. Otherwise it would be possible to write template functions that would call method of non existent classes, isn't it?

In any case, could you review the PR so that we avoid the double inclusion of the same CanvasItem.h file in the .cpp file (I guess it would be included by #include "KSVGCanvas.moc")?

We can merge the PR since it is not a big deal, but I don't think there is a problem in first place. I don't know how the compiler internally instantiate the use of a template, but when it calls the destructor of an object it would call a specific method and the class will have to be well defined at that moment or there would be a compile error. Otherwise it would be possible to write template functions that would call method of non existent classes, isn't it? In any case, could you review the PR so that we avoid the double inclusion of the same CanvasItem.h file in the .cpp file (I guess it would be included by ```#include "KSVGCanvas.moc"```)?
Poster
Collaborator

Ah, "call CanvasItem::~CanvasItem()" is not accurate.

its destructor is not found, unknown base class destructor too, and default destructor cannot be created without its member information, then its destructor will not be called even if it really exists.

Ah, "call `CanvasItem::~CanvasItem()`" is not accurate. its destructor is not found, unknown base class destructor too, and default destructor cannot be created without its member information, then its destructor will not be called even if it really exists.
MicheleC merged commit fb36eda6aa into master 4 months ago
MicheleC deleted branch feat/fix-ksvg-canvas_item-delete 4 months ago
MicheleC added this to the R14.0.12 release milestone 4 months ago
Owner

Thanks. PR merged and backported.

Thanks. PR merged and backported.
The pull request has been merged as fb36eda6aa.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.