As best practices emerge in a post-C++11 world, one of the newer guidelines becoming commonplace is “don’t use raw pointers.” Preferring smart pointers prevents a host of mistakes and provides an additional level of safety and robustness over explicit memory (or refcount) management.
Obviously this doesn’t fit much of VTK’s historical usage, but there are a few ways we can help make it easier to use VTK from projects with more modern coding practices. The vtkNew
, vtkSmartPointer
, and vtkWeakPointer
classes have been updated to use noexcept
when possible and provide move semantics. This enables several new features as discussed here, such as the ability to return vtkNew
from factory methods, store vtkNew
in a std::vector
, safely and efficiently convert between smart pointer types in the same class hierarchy, etc.
That thread also points out some issues with our smart pointers. In particular, this code is legal:
// Example 1:
vtkNew<vtkObject> SomeFactoryMethod();
vtkObject* obj = SomeFactoryMethod(); // (a)
obj->UseObject(); // (b)
This is a bug, since the vtkNew
wrapper is destroyed at the end of line (a)
(decrefing and freeing the object), and then the use of obj
in (b)
does work on a deleted object.
We could just disable the implicit conversion from vtkNew<T>
to T*
, but that will break code such as:
// Example 2
vtkNew<vtkDoubleArray> data;
fieldData->AddArray(data);
which is something that people (including myself) really like the convenience of.
Another work around would be to disable the implicit cast from vtkNew<T>
to T*
only for temporary rvalues by deleting the rvalue-ref qualified cast operator (See this merge request which does exactly that). This (correctly) causes a compile error for Example 1 while allowing Example 2 to still work. However, now this pattern is broken:
// Example 3
fieldData->AddArray(vtkNew<vtkDoubleArray>{});
This should work, but will not longer compile with the rvalue-qualified cast disabled.
What I see as the best path forward here would be:
- Disable the implicit casts from
vtkNew<T>
toT*
in all cases. - Start overloading or refactoring methods that perform reference counting to take a
vtkSmartPointer<T>
instead of aT*
. (vtkNew<T>
can implicitly convert/move into an appropriatevtkSmartPointer<U>
).
This would provide a lot of benefit: The error from Example 1 would be caught at compile time, the code in Example 2 would still work (once AddArray takes a smart pointer), as would Example 3. Passing a raw pointer to these methods would still work, as the vtkSmartPointer
will be implicitly constructed.
This has the benefit that taking a smart pointer inherently documents that a method manages reference counting, while one taking a raw pointer should be treated with care and might just be borrowing a reference from the caller.
The big problem here is that we would break API by changing these methods or adding overloads, so I don’t expect this exact solution to make it in, but I would like to start a conversation around this problem:
What is the best way to integrate vtkSmartPointer
into our API? Right now vtkSmartPointer
and vtkSetObjectMacro
represent different eras in C++, and we rely on some (often dangerous) tricks to get them working together. How can we make it easier and safer to use these together?