Improve const correctness of vtkAbstractDataArray, vtkPoints and derived classes

vtkPoints::GetNumberOfPoints(), vtkAbstractDataArray::GetNumberOfTuples are not constant.


This impedes to be const correct,

 void ReadPoints(const vtkPoints *read_only_points)
{
  auto npoints = read_only_points->GetNumberOfPoints(); // Compile error, GetNumberOfPoints() is not constant.
}

Appending const to the functions will do.

vtkIdType GetNumberOfTuples() const;
vtkIdType GetNumberOfPoints() const;

I guess it will be a little painful to update quite a few functions, but const correctness is important, specially when reasoning about code and in multi-thread environments.

Not sure if this has been already discussed, what do you think?

My experience is that const correctness is often an all or nothing decision. If you start adding const keywords then things will break until you make sweeping changes in the entire code base and also in external code that use it.

Const correctness helps with documentation and avoiding some bugs, so it is good (I would not say it is important), but introducing it would be a huge change, which has to be planned carefully. Also note that any change that forces application developers to modify their code is extremely frustrating if there is no proportional gain (significant new features, better performance, etc.).

1 Like

I see the pain @lassoan, thanks for your input.

However, in this case, just adding a const and a non-const version will do, no breaking changes. And to avoid duplications:

vtkIdType GetNumberOfPoints() const
{
  return this->Data->GetNumberOfTuples();
}

vtkIdType GetNumberOfPoints()
{
  return static_cast<const vtkPoints &>(*this).GetNumberOfPoints();
}

If there are no breaking changes then it’s fine.

GetNumberOfPoints may be called at many places, very frequently, so I would not risk any performance impact by adding one more level of indirection. The non-duplicating code is longer and harder to read. So, I would leave the current implementation of the non-const GetNumberOfPoints as is.

1 Like

I see the pain @lassoan, thanks for your input.

However, in this case, just adding a const and a non-const version will do, no breaking changes. And to avoid duplications:

vtkIdType GetNumberOfPoints() const
{
return this->Data->GetNumberOfTuples();
}

vtkIdType GetNumberOfPoints()
{
return static_cast<const vtkPoints &>(*this).GetNumberOfPoints());
}

+1

Several times I’ve been disappointed when I wanted to make some
function in our code const-correct, but could not since it calls some
VTK function on a member, and VTK is not const-correct. I understand
of course that a sweeping refactor of VTK would create a lot of pain,
but I would welcome baby steps like these where feasible. I think in
library code one should always strive to be const-correct, because it
leaves the library user a choice, whereas “everything non-const” does
not.

Elvis

If I had to choose what VTK developers should spend their time with, const correctness would be pretty low on the priority list compared to improving performance or adding features (oriented image data, robust polydata Boolean operations, GPU-accelerated filters, etc.).

Yes absolutely, not disputing that! It’s just a little sad that const was not kept in mind from the get go.

Pablo,

I occasionally work on improving VTK const correctness, including some of the API you mentioned.

See:
https://gitlab.kitware.com/vtk/vtk/merge_requests/1455/diffs?commit_id=3127c6e1388cc3a8ae1244b7a799b17b7ba7cb1e

If you could do a review of that MR, it could be helpful.

Sean

1 Like

Hi @seanm, that’s great!!

Heading there asap, thanks

Somewhat related: Last week, I submitted a merge request to improve const correctness of vtkTriangle member function parameters:

“Add const to vtkTriangle function parameters”
https://gitlab.kitware.com/vtk/vtk/merge_requests/5278

Please have a look! Is there anything more I could do to get it merged?

Update (15 March 2019): @ken-martin just did the merge: https://gitlab.kitware.com/vtk/vtk/commit/08b3fd3e49d040db640b785b6261ef7985d052ef :slight_smile: