# pimpl management good practice

**URL:** <https://discourse.vtk.org/t/pimpl-management-good-practice/7582>\
**Category:** Development\
**Created:** [January 6, 2022, 7:54pm UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582 "2022-01-06T19:54:51Z")\
**Posts on this page:** 20\
**Page:** 1

<div class="post-metadata">

**Author:** ![mwestphal](https://discourse.vtk.org/user_avatar/discourse.vtk.org/mwestphal/32/19_2.png) [@mwestphal](https://discourse.vtk.org/u/mwestphal)\
**Post date:** [January 6, 2022, 7:54pm UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/1 "2022-01-06T19:54:51Z")

</div>

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
> 
> ```auto
> class vtkInternals;
> class vtkFilter public vtkObject
> {
> 
> vtkFilter()
> ~vtkFilter()
> private:
> vtkInternals* Internals
> }
> 
> ```
> 
> ```auto
> class vtkInternals 
> {
> int foo;
> }
> 
> vtkFilter::vtkFilter()
> {
> this->Internals = new vtkInternals();
> }
> 
> vtkFilter::~vtkFilter()
> {
> delete this->Internals;
> }
> 
> ```
> 
> 1. std
> 
> ```auto
> #include <memory>
> 
> class vtkInternals;
> class vtkFilter public vtkObject
> {
> vtkFilter()
> ~vtkFilter()
> private:
> std::shared_ptr<vtkInternals*> Internals
> }
> 
> ```
> 
> ```auto
> class vtkInternals 
> {
> int foo;
> }
> 
> vtkFilter::vtkFilter()
> {
> this->Internals.reset(new vtkInternals());
> }
> 
> vtkFilter::~vtkFilter() = default;
> 
> ```
> 
> 1. vtk
> 
> ```auto
> #include "vtkNew.h"
> 
> class vtkInternals;
> class vtkFilter public vtkObject
> {
> vtkFilter()
> ~vtkFilter()
> private:
> vtkNew<vtkInternals> Internals
> }
> 
> ```
> 
> ```auto
> 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.

---

<div class="post-metadata">

**Author:** ![utkarshayachit](https://discourse.vtk.org/user_avatar/discourse.vtk.org/utkarshayachit/32/379_2.png) [@utkarshayachit](https://discourse.vtk.org/u/utkarshayachit)\
**Post date:** [January 6, 2022, 7:59pm UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/2 "2022-01-06T19:59:17Z")

</div>

I’d recommend 4th:

```auto
////-------------------------
/// 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.

---

<div class="post-metadata">

**Author:** ![will.schroeder](https://discourse.vtk.org/user_avatar/discourse.vtk.org/will.schroeder/32/233_2.png) [@will.schroeder](https://discourse.vtk.org/u/will.schroeder)\
**Post date:** [January 6, 2022, 7:58pm UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/3 "2022-01-06T19:58:56Z")

</div>

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.

---

<div class="post-metadata">

**Author:** ![mwestphal](https://discourse.vtk.org/user_avatar/discourse.vtk.org/mwestphal/32/19_2.png) [@mwestphal](https://discourse.vtk.org/u/mwestphal)\
**Post date:** [January 6, 2022, 8:01pm UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/4 "2022-01-06T20:01:17Z")

</div>

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.

---

<div class="post-metadata">

**Author:** ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)\
**Post date:** [January 6, 2022, 10:41pm UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/5 "2022-01-06T22:41:05Z")

</div>

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()`.

---

<div class="post-metadata">

**Author:** ![toddy](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/t/edb3f5/32.png) [@toddy](https://discourse.vtk.org/u/toddy)\
**Post date:** [January 7, 2022, 12:23am UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/6 "2022-01-07T00:23:36Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)\
**Post date:** [January 7, 2022, 3:01am UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/7 "2022-01-07T03:01:09Z")

</div>

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.

---

<div class="post-metadata">

**Author:** ![toddy](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/t/edb3f5/32.png) [@toddy](https://discourse.vtk.org/u/toddy)\
**Post date:** [January 7, 2022, 3:06am UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/8 "2022-01-07T03:06:18Z")

</div>

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](https://gitlab.kitware.com/vtk/vtk/-/merge_requests/8784/diffs#7c6cc039d60c1450d502a435418cb5e4ebf5b51c)

---

<div class="post-metadata">

**Author:** ![utkarshayachit](https://discourse.vtk.org/user_avatar/discourse.vtk.org/utkarshayachit/32/379_2.png) [@utkarshayachit](https://discourse.vtk.org/u/utkarshayachit)\
**Post date:** [January 7, 2022, 3:35am UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/9 "2022-01-07T03:35:58Z")

</div>

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.

---

<div class="post-metadata">

**Author:** ![toddy](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/t/edb3f5/32.png) [@toddy](https://discourse.vtk.org/u/toddy)\
**Post date:** [January 7, 2022, 3:40am UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/10 "2022-01-07T03:40:27Z")

</div>

> [@utkarshayachit](#):
>
> The example you point to is not strictly PIMPL

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

> [@utkarshayachit](#):
>
> Independently, in your example it’s not clear to me what benefit you get by allocating `std::vector` on stack.

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

---

<div class="post-metadata">

**Author:** ![mwestphal](https://discourse.vtk.org/user_avatar/discourse.vtk.org/mwestphal/32/19_2.png) [@mwestphal](https://discourse.vtk.org/u/mwestphal)\
**Post date:** [January 7, 2022, 7:56am UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/11 "2022-01-07T07:56:31Z")

</div>

> [@ben.boeckel](#):
>
> 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()`.

Most definitely.

@utkarshayachit unique\_ptr impl seems the most reasonnable one.

---

<div class="post-metadata">

**Author:** ![codeling](https://discourse.vtk.org/user_avatar/discourse.vtk.org/codeling/32/3792_2.png) [@codeling](https://discourse.vtk.org/u/codeling)\
**Post date:** [January 7, 2022, 9:27am UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/12 "2022-01-07T09:27:36Z")

</div>

Minor addition on `unique_ptr`: The C++ Core guidelines recommend to [use `make_unique()` to make `unique_ptr`s](http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-make_unique) instead of explicit allocation with `new` (mainly for exception safety - though in this particular case this is probably not relevant).

---

<div class="post-metadata">

**Author:** ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)\
**Post date:** [January 7, 2022, 12:26pm UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/13 "2022-01-07T12:26:26Z")

</div>

> [@codeling](#):
>
> The C++ Core guidelines recommend to [use `make_unique()` to make `unique_ptr`s](http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-make_unique) instead

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.

> [@toddy](#):
>
> An example is better than an explanation. The vector instance is allocated on the stack.

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

---

<div class="post-metadata">

**Author:** ![utkarshayachit](https://discourse.vtk.org/user_avatar/discourse.vtk.org/utkarshayachit/32/379_2.png) [@utkarshayachit](https://discourse.vtk.org/u/utkarshayachit)\
**Post date:** [January 7, 2022, 12:48pm UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/14 "2022-01-07T12:48:27Z")

</div>

> [@toddy](#):
>
> How? The strings added to the vector are allocated on the heap and need to be freed when the _vtkPythonArgsConverter_ instance is destroyed.

ref: [PImpl - cppreference.com](https://en.cppreference.com/w/cpp/language/pimpl)  
`"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.

---

<div class="post-metadata">

**Author:** ![toddy](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/t/edb3f5/32.png) [@toddy](https://discourse.vtk.org/u/toddy)\
**Post date:** [January 7, 2022, 7:41pm UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/15 "2022-01-07T19:41:33Z")

</div>

Ugh. Ok sorry. I was confusing this with RAII. I really should have looked up that acronym. 🤦‍♂️

---

<div class="post-metadata">

**Author:** ![mwestphal](https://discourse.vtk.org/user_avatar/discourse.vtk.org/mwestphal/32/19_2.png) [@mwestphal](https://discourse.vtk.org/u/mwestphal)\
**Post date:** [January 9, 2022, 11:21am UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/16 "2022-01-09T11:21:29Z")

</div>

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](https://gitlab.kitware.com/vtk/vtk/-/commit/2b3c7a6ec389f29e06a229f68200951bf1d54514)

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

---

<div class="post-metadata">

**Author:** ![will.schroeder](https://discourse.vtk.org/user_avatar/discourse.vtk.org/will.schroeder/32/233_2.png) [@will.schroeder](https://discourse.vtk.org/u/will.schroeder)\
**Post date:** [January 10, 2022, 11:36am UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/17 "2022-01-10T11:36:29Z")

</div>

Sounds good to me.

---

<div class="post-metadata">

**Author:** ![mwestphal](https://discourse.vtk.org/user_avatar/discourse.vtk.org/mwestphal/32/19_2.png) [@mwestphal](https://discourse.vtk.org/u/mwestphal)\
**Post date:** [January 11, 2022, 12:07pm UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/18 "2022-01-11T12:07:36Z")

</div>

I’ve edited my initial post

---

<div class="post-metadata">

**Author:** ![estan](https://discourse.vtk.org/user_avatar/discourse.vtk.org/estan/32/460_2.png) [@estan](https://discourse.vtk.org/u/estan)\
**Post date:** [January 14, 2022, 8:00am UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/19 "2022-01-14T08:00:43Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![lgivord](https://discourse.vtk.org/user_avatar/discourse.vtk.org/lgivord/32/3543_2.png) [@lgivord](https://discourse.vtk.org/u/lgivord)\
**Post date:** [June 5, 2025, 1:21pm UTC](https://discourse.vtk.org/t/pimpl-management-good-practice/7582/20 "2025-06-05T13:21:49Z")

</div>

Sorry to reopen this thread 3 years later, I plan to update the `coding_convention` of VTK to add the standard way to implement a PIMPL in VTK.

I also have a potential improvement for the Utkarsh approach. It concerns the `vtkInternals` class which could be in his own header/source file and these files could be in a `Private` folder. We will have something like that:

```c++
////-------------------------
/// vtkClass.h

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

class vtkClassInternals;

class vtkClass : public vtkObject
{

private:
  std::unique_ptr<vtkClassInternals> Internals;
}

////-------------------------
/// vtkClass.cpp

#include "Private/vtkClassInternals.h"

vtkClass::vtkClass()
  : Internals(new vtkClass::vtkInternals())
{
  this->Internals = std::make_unique<vtkClassInternals>();
}

vtkClass::~vtkClass() = default;

```

note that this is what it is already done in the WebGPU module to separate internals stuff which will not be part of the public API.

wdyt?

FYI @alexy.pellegrini @nicolas.vuaille @jaswantp

[Next page](https://discourse.vtk.org/t/pimpl-management-good-practice/7582.md?page=2)