VTK_NEWINSTANCE macro is available in Slicer's VTK version, so we could mark factory methods return object instance that does not require "UnRegister".

There are only 13 Python files in Slicer core that would need to be updated, so that's very easy.

Probably a number of extensions are impacted as well, which is not very nice (the extra UnRegister will cause Slicer to crash), but Slicer5 would be a good opportunity for making a breaking change like this.

@jcfr @pieper @cpinter @Sunderlandkyl @fedorov what do you think?

Yes, makes sense to do the breaking change now.

I'd like to add that this memory management information from the wiki never made it to readthedocs. While factory method support is added to the python side, it might be good to transfer an updated version of the information from that page into readthedocs.

Less related, on the C++ side: there is nothing in the API documentation for "factory functions" such as vtkMRMLScene::CreateNodeByClass or vtkMRMLStorableNode::CreateDefaultStorageNode to indicate that the caller owns the returned resource and needs to manage it.

@ebrahimebrahim it would be great if you could copy the content to https://github.com/Slicer/Slicer/tree/master/Docs/developer_guide. Probably a new memory_management.md file could be added. Once that's done I would update the wiki page to redirect to the readthedocs page. There are MediaWiki to Markdown converters, but they tend make small mistakes, so for short pages like this it is probably not worth using them.

Adding the VTK_NEWINSTANCE macro should be probably done later, in Slicer-5.1, because the Slicer-5.0 release date is (hopefully) close and it is better to separate this change. Since this API change will make Slicer crash when extensions call UnRegister, we may consider changing the method names and deprecate the old methods. There is some inconsistency anyway between creation of display and storage nodes, so renames would be useful anyway.

@lassoan Do we know exactly what the effects will be in the CreateDefaultStorageNode case, as it is a virtual function? I am assuming that the VTK_NEWINSTANCE will not apply to all derived classes if it is added to the base class, and that it will need to be applied to each derived class as well. Do you know if this is correct?

@Connor-Bowley Good point, I guess it's not vtkMRMLStorableNode::CreateDefaultStorageNode but the derived classes implementing it that need to indicate in their documentation whether the caller owns the resource

When we add VTK_NEWINSTANCE to vtkMRMLStorableNode::CreateDefaultStorageNode we should require (at least by saying so, even if not in an automated manner) that all derived classes do this. Otherwise generic Python code can never be written that calls that function. The Python code would need to handle both cases without any idea which case it was in. If we say a derived class that doesn't have VTK_NEWINSTANCE is ill-formed, then we can shift the responsibility to that class.