vtkSmartPointer and vtkNew in VTK's API. Best practices?

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:

  1. Disable the implicit casts from vtkNew<T> to T* in all cases.
  2. Start overloading or refactoring methods that perform reference counting to take a vtkSmartPointer<T> instead of a T*. (vtkNew<T> can implicitly convert/move into an appropriate vtkSmartPointer<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?

5 Likes

I think it is fine, it is a relatively rare case of API backward incompatibility that should be manageable by proper documentation/migration guide.

To me these look quite big changes and would bring limited benefits. If we decide to completely eliminate raw pointers from all interfaces then it may worth to do it, but as a partial solution, introduced gradually could be quite painful. I would say developers tolerate sweeping API changes in every 5 years or so. For me, the last one was switching to the OpenGL2 backend. Maybe the next one (in 3-4 years) can include getting rid of most raw pointer usage.

They key information that must be documented is not reference counting (that’s an implementation detail), but ownership. Ownership is a simple concept in very specific cases, such as factory methods, but in general they may be very complicated - you cannot document them by using one of 2-3 pointer types. For example, you may use a smart or weak pointer to keep a reference to an object - how would you indicate that in the API? Or, a method might not store the pointer inside its own class but pass the pointer to several other objects that store it in smart pointers - how would you document this using pointer types?

We could add a new macro - vtkSetObjectReferenceMacro or something like that.

As long as all VTK classes derive from vtkObject, internally reference count, and disallow direct invocation of the constructor it seems to be of limited utility to try and make the VTK specific pointer classes behave as if move semantics are necessary. You can only create a VTK class instance as a pointer, returning a vtkNew and moving to a vtkNew or vtkSmartPointer is nice, and offers unique_ptr like behavior.

At the end of the day vtkObject derived classes are primarily pointer based, and if I assign a vtkNew owned object to a vtkSmartPointer its life will be extended beyond the vtkNew even without any move. I am a big fan of using smart pointer classes where reasonable, but without much bigger changes to the VTK inheritance hierarchy it looks like a lot of work and API churn with limited returns. If VTK wants to make bigger changes is it worth reconsidering whether internal reference counting still makes sense, whether we could allow directly calling the constructors/destructors, and relax everything deriving from vtkObject. These are pretty huge changes, the community may not want to take that step, but it would let us use the STL more naturally.

It would be pretty sad to go back to not implicitly casting vtkNew to T*, if we are trying to create a thing to return more safely from factories maybe a fourth class such as vtkUniquePointer could serve that purpose without causing churn in the rest of the API? I think not having that implicit cast limited adoption of an otherwise very useful smart pointer for many years. My $0.42…

To be clear, I’m definitely not in favor of removing the lvalue implicit cast without updating the API to use smart pointers appropriately. Nobody wants to go back to needing .Get() everywhere. That would be the “best” route in the sense that raw pointers would be eliminated from our API and the risks of bad/unsafe implicit conversions would be eliminated. But it would also be an incredibly disruptive change, so I don’t really expect it to happen.

These days, I pretty much treat vtkNew like unique_ptr. The functionality is nearly the same and it’s useful in many of the same contexts.

So far, it seems the most agreeable solution is to just disable the rvalue implicit casts and keep things mostly the way they are. I think we can live with just needing to use Get when pulling a pointer out of a temporary smart pointer.

3 Likes

I posted in the other thread, glad to hear you aren’t wanting to remove implicit casts entirely. I wonder if the generalization is going to make it harder for new users, and posed the same question over there. I don’t feel overly invested, but if we want to treat vtkNew like unique_ptr I wonder if we might want to add a fourth smart pointer named as such with the narrower scope.

At the end of the day some of us like RAII, short, concise code with minimal repetition that clearly conveys our intent. VTK forces all instances to be pointers, and as such there will always be some level of nudging things around to fit a more familiar STL C++11 feel unless we make the huge leap to embrace the language as it is now and radically alter API.

I fought pretty hard to get vtkNew in over a period of a year before auto was even a possibility to replace ugly VTK_CREATE macros and stop having huge lines that discouraged safe practices by their verbosity and repetition. We should keep pushing for more modern APIs, improved best practices, and ideally simple mappings to language features if we want to attract more people to VTK’s C++ API.

Things like improved memory handling, const-correctness, returning values where possible, pass by value or const ref, not exposing internal data, thread safety, asynchronous execution and more represent significant challenges/questions on how much change is OK. Also how do we manage change, several disruptive changes every 6-12 months is likely to drive users away.

1 Like

We could do a developer survey to see how much change VTK users are willing to tolerate in exchange for modernization of the library.

Can we please leave vtkSmartPointer as is. I see errors in VTK and the VTK Examples Project. In Filters/FlowPaths/vtkStreaklineFilter.CXX this->Filter->Output->SetLines(vtkSmartPointer::New());

In VTKExamples/src/Cxx/RenderMan/PolyDataRIB.cxx VTKExamples/src/Cxx/Visualization/PointDataSubdivision.cxx

Why do we need this change?

Why do we need this change?

This is discussed at length in the post and comments above. The tldr version is “VTK is a pain to use with modern C++. How can we smooth the rough edges?”

Sorry, I am far from a c++ expert. From a user point of view, the use of vtkSmartPointer in those examples seems natural.

But I’ll defer to the purists…

I agree that this is bad: ```
vtkSmartPointer data = vtkDoubleArray::New();

Ah, I see. I can explain it a bit more if the prior discussion was too advanced. It’s not so much a purist issue as it is a practical safeguard against dangerous coding patterns that introduce subtle and difficult-to-spot bugs.

In a nutshell, allowing temporary RAII containers to implicitly provide access to their resources is a dangerous design, as the pointer obtained from the implicit cast will be released at the end of the line. It’s similar to the reason std::string won’t implicitly cast to const char*, for example:

std::string someFunction();

const char *timebomb = someFunction().c_str();

where timebomb is a pointer to freed memory.

There are two ways to fix your snippet with the proposed change, both are simple fixes:

  1. Explicitly Get() the pointer from the temporary (equivalent to calling c_str()). This says “I know there is a conversion here, and I’ve checked that it’s safe”.
Output->SetLines(vtkSmartPointer<vtkCellArray>::New().Get())
  1. Store the temporary in an named lvalue, and the implicit cast will still work:
auto lines = vtkSmartPointer<vtkCellArray>::New();
Output->SetLines(lines);

While these might seem contrived since SetLines does reference counting, disabling the implicit cast for rvalues prevents more subtly buggy code like

vtkSmartPointer<vtkCellArray> someFunction();

vtkCellArray *oops = someFunction();
oops->Register(); // crash; the vtkSmartPointer dtor freed
                  // the vtkCellArray in the previous line.

from compiling. Like the std::string example, the oops pointer references freed memory and will cause problems, possibly much later in execution and in a different context after getting passed around. But by disabling the implicit cast for rvalues, we get an immediate compile error at the buggy line, instead of a phantom segfault in a random spot later in the program at runtime.

  1. Is not intuitive.
  2. Is ok,

Re: @cryos, I’m not opposed to adding a unique_ptr equivalent if vtkNew is intended for only stack-based usage. This would imply that holding a vtkNew as a struct field is bad practice, as it implicitly disables move semantics for the owning struct:

struct Foo
{ // Foo(Foo&&) will not be available by default.
  vtkNew<...> Bar;
};

which was my main motivation for adding move semantics to the smart pointers. The new async ParaView refactoring will be making use of C++11 and C++14 paradigms, including move semantics, and allowing these smart pointers (and containers that hold them) to support moves will simplify that work considerably.

That said, other than the name not quite fitting right, I’m not sure why we would need to prevent vtkNew from having these extra abilities, and all of the problems we’re discussing today about vtkNew would still apply to a vtkUniquePtr.

Also, I forgot to mention, I wouldn’t discount the benefit from bypassing reference counting by moving instead of copying. Calling vtkObjectBase::Register is a lot of jumps and virtual calls that will slow things down and trash the instruction cache, vs a move which just copies 8 bytes and sets the source to nullptr.

Plus, these move operations not only bypass reference counting completely, but they also enforce noexcept semantics, which enable a ton of compiler optimizations and allow things like vtkNew to be stored in e.g. std::vector, which is a nice added benefit.

These comments are more general but strongly support best practice.

I think we should always take advantage of compiler optimizations and newer ways of doing things. When I first started using VTK in 2000 there was another visualisation suite (IBM?) that we looked at, however for many reasons, VTK was better, mainly because it was open source and many new features and fixes were happening. From this period onwards VTK became the dominant Visualisation Suite, mainly because of the way Kitware developed it and because of the fact it was open source. A quick look at the literature finds this one paper Comparing and evaluating computer graphics and visualization software with this comment “The VTK prevails on top in many of the aspects we compared and evaluated.” So VTK has to maintain its competitive advantage and to do this it must change and evolve getting better and better.

VTK has to evolve to more fully utilise C++11 and in the future C++14/17… Compilers are getting smarter at optimising and there are newer programmers who understand and want to use the features of modern C++. So it is not a bad thing to evolve VTK to take advantage of more modern C++. Sure old code will break and need fixing but it should run better and be easier to maintain. Also this process often uncovers subtle bugs.

VTK 8.90 is a moving feast at present but we are keeping up with the changes in VTKExamples in respect to C++! Mostly through doing something like this where changes are needed:

#include <vtkVersion.h>

#if VTK_MAJOR_VERSION > 8 || VTK_MAJOR_VERSION == 8 && VTK_MINOR_VERSION >= 90
#include "vtkShaderProperty.h"
#endif
...
 // Modify the vertex shader to pass the position of the vertex
#if VTK_MAJOR_VERSION > 8 || VTK_MAJOR_VERSION == 8 && VTK_MINOR_VERSION >= 90
  vtkShaderProperty* sp = actor->GetShaderProperty();
  sp->AddVertexShaderReplacement(
#else
  mapper->AddShaderReplacement(
    vtkShader::Vertex,
#endif

However for Python, we cannot do this.

The VTKExamples are also a good testing ground for new features.

1 Like

Why is this kind of dual-compatibility not possible for Python examples? The required version can be checked against the vtkVersion class.

Yes, writing code that is compatible with different VTK versions is just as easy to do in Python as in C++. For example:

if vtk.VTK_MAJOR_VERSION <= 5:
  threshold.SetInput(allSegmentsLabelmapNode.GetImageData())
else:
  threshold.SetInputData(allSegmentsLabelmapNode.GetImageData())

Andras,

Thanks for that, I’m aware of that! I must have been thinking of something else probably how to handle different imports for different versions of Python!

Just a minor comment:

This link seems to not point anywhere. I tried clicking it and hovering over it but it leads to nowhere using chrome and firefox. Could you re-post the link?
Thanks.

Looks like a stray character broke the link – should be working now!

1 Like