# About avoiding public virtual method in VTK

**URL:** <https://discourse.vtk.org/t/about-avoiding-public-virtual-method-in-vtk/12240>\
**Category:** Development\
**Created:** [August 29, 2023, 1:08am UTC](https://discourse.vtk.org/t/about-avoiding-public-virtual-method-in-vtk/12240 "2023-08-29T01:08:12Z")\
**Posts on this page:** 6\
**Page:** 1

<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:** [August 29, 2023, 1:08am UTC](https://discourse.vtk.org/t/about-avoiding-public-virtual-method-in-vtk/12240/1 "2023-08-29T01:08:12Z")

</div>

Mod note: This was initially an answer of:  
[https://discourse.vtk.org/t/member-visibility-when-writing-new-vtk-classes/12208/8](https://discourse.vtk.org/t/member-visibility-when-writing-new-vtk-classes/12208/8)

In addition I think all **public** methods should be non-virtual and call **protected** virtual methods if necessary.

---

<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:** [August 29, 2023, 7:07am UTC](https://discourse.vtk.org/t/about-avoiding-public-virtual-method-in-vtk/12240/2 "2023-08-29T07:07:23Z")

</div>

> In addition I think all **public** methods should be non-virtual and call **protected** virtual methods if necessary.

While I agree with the intent, the fact that vtkGetMacro and vtkSetMacro are virtual make this not possible atm.

---

<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:** [August 29, 2023, 3:25pm UTC](https://discourse.vtk.org/t/about-avoiding-public-virtual-method-in-vtk/12240/3 "2023-08-29T15:25:54Z")

</div>

I’m not sure what that accomplishes; devirtualization is the ideal optimization to hit in such cases and hiding the virtual call seems like it would pessimize it.

---

<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:** [August 30, 2023, 12:09am UTC](https://discourse.vtk.org/t/about-avoiding-public-virtual-method-in-vtk/12240/4 "2023-08-30T00:09:09Z")

</div>

Avoiding virtualisation is a different discussion from the point I was making.

> **[Public Virtual Methods: A Bad Idea](https://blog.spatial.com/3d-software-development-kits/public-virtual-methods-bad-idea)**
>
> The answer for public methods is easy: if you think of the class as providing a set of services, it’s the clients of the services, i.e. the users of the class.

---

<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:** [August 30, 2023, 12:41pm UTC](https://discourse.vtk.org/t/about-avoiding-public-virtual-method-in-vtk/12240/5 "2023-08-30T12:41:31Z")

</div>

The issue I see with it is that it doesn’t scale for VTK. If one should not have public `virtual` methods for the reasons presented, the logical conclusion (AFAICS) is this monstrosity:

```c++
class Base {
public:
  void Access();
private:
  virtual void AccessImpl();
};

class Middle : public Base {
protected:
  void AccessImpl() final;
private:
  virtual void AccessImplForMiddle();
};

class Next : public Middle {
protected:
  void AccessImplForMiddle() override;
};

```

Without this, `Next` could override `AccessImpl` and forget to call `Middle::AccessImpl`: the problem presented in the introduction of the blog post.

Given that VTK has class hierarchy depths often 5-deep, but sometimes even more, the stack of new `virtual` methods is just not going to work well. One also needs to either prepare this for every `virtual` method in case logic needs to be added in the future or go and find all callers and update them to use the new `virtual` method added in between.

FWIW, I think it’s far easier to just remember to call `this->Superclass::SameMethodName` in the implementation because that works in all cases. Is it more work? Yes. But since when has C++ ever considered “more work” on the part of the programmer/maintainer/reviewer a fatal problem?

---

<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:** [September 2, 2023, 3:45am UTC](https://discourse.vtk.org/t/about-avoiding-public-virtual-method-in-vtk/12240/6 "2023-09-02T03:45:23Z")

</div>

> [@ben.boeckel](#):
>
> If one should not have public `virtual` methods for the reasons presented, the logical conclusion (AFAICS) is this monstrosity:

Indeed that is a monstrosity but you have only focused on one aspect of the article I linked and that aspect is not one I was particularly advocating for. I’m fine with **protected** virtual methods, rather than **private** virtual methods, as stated in my original comment, and calling `this->Superclass::MethodNameImpl()`, or not, depending on the situation. So the VTK deep hierarchy of virtual method overrides need not be drastically altered.

There are other advantages to enforcing non-virtual public methods.

- Because flow passes through the base class, standard input checking can be implemented in the base class method.
- Pre- and post-operation contract checks can be performed in the base class method. This is especially important for the post-operation checks, because it can be used to ensure that implementers didn’t get subtle required behaviors wrong.
- The virtual method doesn’t need a base class implementation, so it can be declared pure (forcing implementers to realize that the need to write a method) without confusing implementers with the “pure virtual methods are allowed to have implementations” subtlety.

Furthermore the virtual method can be customised to pass parameters to descendant classes without affecting the public API.

Replacing

```auto
class vtkMyClass
{
public:
  virtual void SomeMethod();

protected:
  int myMember;
};

```

with

```auto
class vtkMyClass
{
public:
  void SomeMethod();

protected:
  virtual void SomeMethodImpl(int myMember);

private:
  int myMember;
};

```

narrows the visibility of the class data to certain methods and clarifies the dependencies of the virtual overrides.