vtkNew and vtkSmartPointer

Development
1

Folks,

We have had on and off discussions about which to use. Both can be used pretty much interchangeably.
vtkNew is not friendly with some STL containers. But this is not a major issue for vtk since STL containers are not used that often in VTK. We do use them both in the VTK Examples project. We do not mix them.

Some consider vtkSmartPointer is too verbose.

I want to point out that since we now require c++11, we. can use the auto keyword:

auto actor = vtkSmartPointer::New();

I think which you use a personal choice.
Bill

1 Like
2

I do not think that is the case. Afaik, you can’t return a vtkNew.

3

Mathieu, I believe you are virrrct.

Bill

4

Good point Bill, I never thought of using:

auto actor = vtkSmartPointer::New();

I will now! That looks so much easier to read!
For those interested I tried to provide a “guideline” in AmbientSpheres wrt vtkNew and STL containers, please feel free to improve on it.

Regards
Andrew

5

The actual syntax is:

auto actor = vtkSmartPointer<vtkActor>::New()

I agree it is shorter and not as redundant as:

vtkSmartPointer<vtkActor> actor = vtkSmartPointer<vtkActor>::New()

But still not as short, simple, and foolproof as:

vtkNew<vtkActor> actor;

I find both vtkNew and vtkSmartPointer very useful and choose vtkNew whenever it is possible and vtkSmartPointer otherwise. Most new users seem to only use VTK via Python anyway, while long-time VTK users got to do things certain way, so maybe there is not much to do here. Perhaps the examples could be updated to use more smart pointers instead of manual object creation/deletion.

1 Like
6

Sorry for the typo…

All of the VTK Project examples use either vtkNew or vtkSmartPointer.

1 Like
7

Coincidentally, I just did a merge request, replacing New() and Delete() by vtkNew, within vtkCurvatures.cxx:

https://gitlab.kitware.com/vtk/vtk/commit/0766e230b2702cdfe96582b702d25bfb32484751?merge_request_iid=5284

vtkNew appears very convenient to me, but please tell me if you prefer vtkSmartPointer in this case. I won’t mind amending the commit.

8

vtkNew is fine.

1 Like
9

This used to be true, until a few weeks ago:

https://gitlab.kitware.com/vtk/vtk/merge_requests/5223

Now vtkNew has move semantics, and can be returned:

vtkNew<vtkDataSet> ObjectFactory()
{
  vtkNew<vtkPolyData> pd;
  Configure(pd);
  return pd;
}

void Work()
{
  vtkSmartPointer<vtkObject> object; // empty

  { // Create new scope
    // Implicitly moves the returned objects into dObj1 and dObj2;
    vtkNew<vtkDataObject> dObj1 = ObjectFactory();
    vtkNew<vtkDataObject> dObj2 = ObjectFactory();

    // The dangling returned pointer is immediately destroyed properly:
    ObjectFactory();

    // dObj1 (vtkNew) is moved into object (vtkSmartPointer) 
    // and set to nullptr. dObj2 remains valid.
    object = std::move(someTrueCondition ? dObj1 : dObj2); 
  }  // dObj2 is destroyed
  
  Foo(object); // do some work with dObj1

} // dObj1's original data is freed as object is destroyed

The new standard guarantees the we’ll elide the copy via NRVO in ObjectFactory(), and since the type-converting move constructors are present the type cast works as expected. The returned object will have a reference count of one and will clean itself up if dangled, or else must be moved into a different vtkNew or vtkSmartPointer, possibly converting type up the inheritance chain.

The move semantics also mean that vtkNew pointers can now be stored in containers like std::vector, since the containers are now guaranteed by the standard to move elements instead of copy them when noexcept move constructors are available. They’ll work as desired during vector reallocation and free themselves when the vector is destroyed. There are some restrictions – the std::vector of vtkNew becomes move-only as well, but under the ownership semantics implied by vtkNew, this is still useful.

All of the smart pointers got a little C++11 polish with that branch and learned some new tricks. The major difference remaining between New and Smart is vtkNew has both move-assignment and copy-assignment disabled, meaning that once a pointer goes into a vtkNew, it cannot change. It will go to nullptr if moved out, but it cannot be moved into or otherwise replaced once constructed.

IMO they’re both still useful to have because of convention – vtkNew means “I own this pointer, which must remain the same for my lifetime”, vtkSmartPointer means “I use this pointer, which came from somewhere else and might change,” which is useful to know at a glance when maintaining code.

5 Likes
10

It’s great that factory method API ambiguities can be finally resolved (the problem was that after returning a simple pointer the user must remember to delete manually or take over ownership; when returning vtkSmartPointer, user may assign it to a regular pointer, resulting in a dangling pointer). I guess a returned vtkNew can only be assigned to a vtkNew, so it cannot be misused.

Does Python-wrapping work well with vtkNew<> return value?

1 Like
11

Right, it must be used to move-construct a vtkNew in the caller’s frame, or moved/copied into a vtkSmartPointer. Either way, it’s possible to use VTK now without needing to manually manage reference counting.

I just noticed a possible issue with this pattern and the implicit casts to T*:

vtkNew<vtkObject> SomeFactoryMethod();

void bad()
{
  vtkObject *obj = SomeFactoryMethod(); // error, obj dangles
  obj->Print(std::cout);
}
void good()
{
  vtkNew<vtkObject> obj = SomeFactoryMethod(); // correct
  obj->Print(std::cout);
}

Currently, both compile and bad() results in a subtle runtime bug. I have a patch coming that disables the implicit cast from vtkNew rvalues so that this bug will be caught by the compiler.

Good question! I’m not sure. While the vtkNew factory pattern is possible now, I’m not aware of it being used in VTK. @dgobbi might know offhand whether the Python layer would do the right thing here. I’d avoid using this pattern in public APIs until we know that the python support for vtkNew can handle it.

1 Like
12

Would this still work?

vtkNew<vtkPolyData> pd;
somefilter->SetInputData(pd);

Requiring using .GetPointer() for vtkNew’d smart pointers was terrible and we should not bring it back. Maybe we need to introduce a new type, which would store a pointer inside privately but it could only be accessed by assigning it to a smart pointer (vtkSmartPointer or vtkNew). This type would only be used for returning a value from factory methods.

13

That will still work. The check is for rvalue references only:

void ok()
{ 
  vtkNew<vtkPolyData> pd;
  // argument is an lvalue, implicit cast to T* allowed
  filter->SetInputData(pd);
}

void not_ok()
{
  // argument is rvalue, implicit cast to T* disallowed
  filter->SetInputData(vtkNew<PolyData>{}); // <-- compile error
}

void ok_again()
{
  // Argument is raw pointer, no implicit cast needed.
  // The explicit Get() call implies that the author has thought
  // about reference counting in the callee and is aware that this is a
  // boundary between automatic and manual reference counting.
  filter->SetInputData(vtkNew<vtkPolyData>{}.Get());
}

So basically, the implicit cast only works when the vtkNew object is not a temporary rvalue that would be destroyed at the end of the statement. If someone really wants to get the pointer out of the temporary, they can use Get(), but that’s a “here be dragons” operation and requiring the extra call prevents mistakes.

1 Like
14

The merge request is here if anyone wants to take a look: https://gitlab.kitware.com/vtk/vtk/merge_requests/5308

15

Looks nice, when the dust settles, it would be nice to have an article and guidelines as to the use of vtkNew/vtkSmartPointer/vtkWeakPointer since this is now a bit dated: A Tour of VTK Pointer Classes also I attempted to introduce a section in the VTK TextBook, section: 3.11.2 VTK Pointer Classes. The original text book was written well before smart pointers so I rewrote most of the code in the PDF version of the textbook to mainly use vtkNew, sometimes vtkWeakPointer (e.g. Fig 3-25) reserving vtkSmartPointer for uses like in std::vector.

16

I like this comment, this is what I mean about clarifying the roles of vtkNew and vtkSmartPointer.

17

Agreed. Updating the docs / textbook and making a new “Best Practices” blog post is definitely in order as we iron out the remaining issues.

18

Python support for vtkNew is absent right now, but it would be easy to add. The wrappers just have to capture the returned vtkNew object, Get() the pointer, construct a Python object, and then let the vtkNew go out of scope. This would require around 20 new lines of code at most.

19

Nice discussion and work.

Are there any concrete plans to work through the code base and make everything consistent / uniform ? We seem to be good at identifying solutions but often they are only partially implemented :slight_smile: which can lead to frustrated users (and developers for that matter).

1 Like
20

The current implementation of vtkNew and vtkSmartPointer is already complete. By “making everything consistent / uniform”, do you mean making smart pointers replace raw pointers everywhere? If so, I’ve started another discussion to go over the specific issue of using smart pointers in VTK’s API:

In a perfect world, it’d be good to start using smart pointers in our APIs. But realistically, I don’t see that happening as it would break virtual APIs and/or introduce hidden overload issues for projects that extend VTK’s classes. If we wanted to do that, we might as well rewrite VTK from the ground up and use internal shared_ptrs to manage object lifetimes, like VTK-m does.

With that effort vs. benefit ratio, I more or less expect the end result to be “leave things as they are”, but wanted to start the discussion, gauge interest, and see what possible solutions the community comes up with.