About avoiding public virtual method in VTK

Development
1

Mod note: This was initially an answer of:
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.

2

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.

3

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.

4

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

5

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:

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?

6

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

class vtkMyClass
{
public:
  virtual void SomeMethod();

protected:
  int myMember;
};

with

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.