Should we replace SafeDownCast with dynamic_cast

Development
1

Folks,

SafeDownCast was added to vtk way back when dynamic_cast was not available everywhere. Should we replace SafeDownCast in vtk with dynamic_cast? It is more natural for modern C++ and is well-documented. It may be more efficient although I doubt the current implementation is not a significant resource consumer.

See:

vtkKochanekSpline::SafeDownCast(this->ParametricSpline->GetXSpline())->SetDefaultTension(value);

versus

dynamic_cast<vtkKochanekSpline *>(this->ParametricSpline->GetXSpline())->SetDefaultTension(value);

It is used in over 1500 vtk files. but the changes could be automated.

Bill

4 Likes
2

I did an interesting experiment. I pushed an MR that replaced the implementation of SafeDownCast to use dynamic_cast. The first problem was using vtkAbstractHyperTreeGridMapper in vtkCellPicker. The hyper class has no subclasses in VTK. Not sure why.

I’m also getting some failed tests. I’ll continue to investigate…

1 Like
3

I like this idea Bill, seems like if we can do it, we should.

4

SafeDownCast is redundant in wrapper code, so no issues there.

5

I was under the impression that there are different options to improve performances of dynamic_cast. (e.g. http://www.stroustrup.com/fast_dynamic_casting.pdf). By keeping SafeDownCast, we let ourselves the option to speedup downcasting some day…

But maybe things have changed with the newests C++, I am not really up to speed here.

6

@jjomier I pushed an MR that replaced SafeDownCast with dynamic_cast. As you suspected, there are performance issues. We are better off keeping DafeDownCast. I’m afraid I wasted some of my not-so-costly time. I am closing the MR.

See these results: https://tinodidriksen.com/2010/04/cpp-dynamic-cast-performance/

Bill

7

Thanks for experimenting @lorensen we now know there are performance issues!

8

Here’s a link to a dashboard that illustrates performance and failure related to my dynamic_cast experiment.

1 Like
9

Did you identify the cause of failure with the dynamic cast on some test machines? It seems odd.

10

There were lots of discussion about this at the hackathon. Way above my level of c++ knowledge.