pimpl management good practice

Development
1

Hi VTK devs.

With the prevalence of pimpl in VTK I wonder what would be the best practice for it:

EDIT: Use the solution: 4th : std::unique_ptr

Summary
  1. Classic
class vtkInternals;
class vtkFilter public vtkObject
{

vtkFilter()
~vtkFilter()
private:
  vtkInternals* Internals
}

class vtkInternals 
{
  int foo;
}

vtkFilter::vtkFilter()
{
  this->Internals = new vtkInternals();
}

vtkFilter::~vtkFilter()
{
  delete this->Internals;
}
  1. std
#include <memory>

class vtkInternals;
class vtkFilter public vtkObject
{
vtkFilter()
~vtkFilter()
private:
  std::shared_ptr<vtkInternals*> Internals
}

class vtkInternals 
{
  int foo;
}

vtkFilter::vtkFilter()
{
  this->Internals.reset(new vtkInternals());
}

vtkFilter::~vtkFilter() = default;
  1. vtk
#include "vtkNew.h"

class vtkInternals;
class vtkFilter public vtkObject
{
vtkFilter()
~vtkFilter()
private:
  vtkNew<vtkInternals> Internals
}

class vtkInternals public vtkObjectBase
{
  int foo;
}

vtkFilter::vtkFilter() = default
vtkFilter::~vtkFilter() = default;

I like 3, it is more VTK style imo and also more concise.

2 Likes
2

I’d recommend 4th:

////-------------------------
/// vtkClass.h

#include <memory> // for std::unique_ptr

class vtkClass : public vtkObject
{

private:
  class vtkInternals;
  std::unique_ptr<vtkInternals> Internals;
}

////-------------------------
/// vtkClass.cpp
class vtkClass::vtkInternals
{
public:
...
};

vtkClass::vtkClass()
  : Internals(new vtkClass::vtkInternals())
{
}

vtkClass::~vtkClass() = default;

BTW, your #3 is incomplete. You’d need vtkStandardNewMacro(...) etc. for vtkInternals which IMO just is an overkill.

4 Likes
3

This is great Mathieu, thanks for getting the discussion going. I’m going to mull on this a bit - I’ve probably implemented all of the above :-), but I agree some standardization would be good.

4

Good point about the incompletness of the class.

About 4. I wish there was some sort of std::new to avoid the constructor non default. still a good one.

5

Please don’t use shared_ptr, but unique_ptr instead. There are zero reasons why these classes should escape and be cooperatively shared around. Also, just do Internals(new vtkInternals) instead of .reset().

6

What about when the Internals class is a container? It can be allocated on the stack.

7

Why would the class be in a container? Do you have some use case in mind? Note that most containers require a full declaration (for sizeof) which kind of defeats the point.

8

An example is better than an explanation. The vector instance is allocated on the stack. https://gitlab.kitware.com/vtk/vtk/-/merge_requests/8784/diffs#7c6cc039d60c1450d502a435418cb5e4ebf5b51c

9

I think that is slightly orthogonal. The example you point to is not strictly PIMPL. So it shouldn’t’ really be affected by this.

Independently, in your example it’s not clear to me what benefit you get by allocating std::vector on stack. It’s contents are most certainly not on stack. Hence, you could adopt PIMPL there and then std::unique_ptr approach proposed here would work just fine. Note, you don’t have to. The proposal here is not to require PIMPL, but come up with a standard practice should one choose to do so in their VTK classes.

10

How? The strings added to the vector are allocated on the heap and need to be freed when the vtkPythonArgsConverter instance is destroyed.

It saves having to create and destroy the vector instance explicitly.

11

Most definitely.

@utkarshayachit unique_ptr impl seems the most reasonnable one.

12

Minor addition on unique_ptr: The C++ Core guidelines recommend to use make_unique() to make unique_ptrs instead of explicit allocation with new (mainly for exception safety - though in this particular case this is probably not relevant).

13

Alas, that is beyond VTK’s C++ requirement. In any case, VTK is already exception-ignorant, so any such exception will surely leave all kinds of other rubble in its wake; a bit of leaked memory is the least of anyone’s worries at that point. But, once we have C++14, yes make_unique all the way.

That’s not PIMPL at all because it is in a public header.

1 Like
14

ref: PImpl - cppreference.com
"Pointer to implementation" or "pImpl" is a C++ programming technique[1] that removes implementation details of a class from its object representation by placing them in a separate class, accessed through an opaque pointer:

You’re not doing that. You are placing the details of the class in the header.

15

Ugh. Ok sorry. I was confusing this with RAII. I really should have looked up that acronym. :man_facepalming:

16

So I think using @utkarshayachit impl is the way to go.

I realize that you have been using this style for a while, see here for example:
https://gitlab.kitware.com/vtk/vtk/-/commit/2b3c7a6ec389f29e06a229f68200951bf1d54514

I will start using that and will push this in my reviews.

2 Likes
17

Sounds good to me.

18

I’ve edited my initial post

19

Late to the party, but in support of The Utkarsh Approach.

1 Like