When should you restrict accessibility to a virtual function in a derived class?

Question

Consider the following code:

class Base
{
public:
    virtual void Foo() {}
};

class Derived : public Base
{
private:
    void Foo() {}
};

void func()
{
    Base* a = new Derived;
    a->Foo(); //fine, calls Derived::Foo()

    Derived* b = new Derived;
//  b->Foo(); //error
    static_cast<Base*>(b)->Foo(); //fine, calls Derived::Foo()
}

I've heard two different schools of thought on the matter:

1. Leave accessibility the same as the base class, since users could use static_cast<Derived*> to gain access anyway.

2. Make functions as private as possible. If users require a->Foo() but not b->Foo(), then Derived::Foo should be private. It can always be made public if and when that's required.

Is there a reason to prefer one or the other?

Hoshi

This design is very counter-intuitive for the reasons you mention. I would advise against it unless you encounter a scenario that can only be solved this way. – Backhanded

If your intent is to restrict direct usage of the derived class (e.g. Factory pattern), then protected or private inheritance is more appropriate way (instead of restricting particular methods) – Redneck

Answer 1

Restricting access to a member in a subtype breaks the Liskov substitution principle (the L in SOLID). I would advice against it in general.

Update: It may "work," as in the code compiles and runs and produces the expected output, but if you are hiding a member your intention is probably making the subtype less general than the original. This is what breaks the principle. If instead your intention is to clean up the subtype interface by leaving only what's interesting to the user of the API, go ahead and do it.

Lampblack

I don't think this break the substitution principle. You can still use a reference to Derived anywhere a reference to Base is required, and it will work just fine. – Backhanded

@BjörnPollex I was going to reply by comment but then realised the answer would benefit from an update. – Lampblack

Do any compilers flag it as a warning? (Just another data point) – Hoshi

+1, however, I'd mention that it may be applied only to public inheritance which usually is a mechanism to model is_a relationship. To restrict direct access to the derived class private or protected inheritance should be use (instead of restricting access to particular members). – Redneck

@Hoshi I doubt it; compilers don't tend to give design advice. In the same way no compiler would warn you about using inheritance where composition is the preferred option. – Lampblack

@BjörnPollex: "can still usa a reference to Derived..." - true, but if you have a template taking that reference without forcing it back to a Base&, or a Base object by value, then you can't pass it a Derived object. These scenarios might not be the home ground for LSP, but the concepts in LSP remain applicable and should be honoured where possible. Hiding the virtual function unnecessarily frustrates CPU/memory tuning using a mix of runtime and compile-time polymorphic mechanisms. – Madeline

@Tony: I'm specifically talking about cases where no consumer has needed to call b->Foo(). If that need arose (eg a template taking that reference without forcing it back to a Base&), then presumably you could change it to public at that time, no? – Hoshi

@Jon: that depends very much on how you package and distribute your code. If some customer calls you up and says, "the need has just arisen", then can you push a new release out of the door fast enough for them? And at least in theory, changing a member function from private to public could break binary compatibility, although I suspect in practice you'll be OK in pretty much any executable format. Given that there's a workaround, I think it's better to either support it or not support it, don't think as though it's available even when it isn't, on the basis that you could add it later. – Hatty

@Steve, +1, I agree that it depends on how you distribute your code. – Hoshi

Answer 2

Not an answer to your original question, but if we are talking about classes design...

As Alexandrescu and Sutter recommend in their 39^th rule, you should prefer using public non-virtual functions and private/protected virtual:

class Base {
public:
    void Foo(); // fixed API function

private:
    virtual void FooImpl(); // implementation details
};

class Derived {
private:
    virtual void FooImpl(); // special implementation
};

Prismatic

You could reasonably put the implementation of Base::Foo right there in the class definition, void Foo() { FooImpl(); }. It's never going to be more than a predictable one-liner, that's the point. – Hatty

@SteveJessop Agree, if FooImpl() is really just an implemetation for Foo. In real life it may be a part of Foo's algorithm with fixed structure but some floating details. And sometimes, corporate coding standards may forbid implementation in class declaration even for one-liners :) – Prismatic

@SteveJessop: Actually, NO! The point is that you can change Foo at will to include pre/post treatment before calling the virtual function. It it was always a one-liner, it would be quite pointless to include an extra wrapper! – Benzophenone

@anxieux: the issue with inlined one-liners is inlining. Any change therefore requires all clients to re-compile. Within the library, it's clearly not an issue, however when you deliver middleware libraries to your users, you want to be able to say "just switch to this new library" rather than "switch to the new library, and oh, you need to recompile and deliver a new version of your libraries too". It's a matter of ABI compatibility. – Benzophenone

@Matthieu: depends on the purpose of the pattern. I assumed it was in order to create a non-virtual public interface. It has the side-effect of allowing pre- and post-code, a sort of half-assed version of weave points, but that's independent of the goal of removing virtual-ness from public interfaces. Sutter remarks "Actually it's a more restricted idiom with a form similar to that of Template Method ... I've switched to calling the idiom the Non-Virtual Interface Idiom". My emphasis, I think it's risky to start out only intending NVI, but then add behavior to the base function. – Hatty

So, it is not pointless to create an extra wrapper, the point is to separate the specification of the public interface (Foo), i.e. the interface presented by Base to its users, from the specification of how subclasses should customize the behavior of the base class (FooImpl), i.e. the interface presented by subclasses of Base to Base itself. See GoTW18, gotw.ca/publications/mill18.htm. I do agree that if Foo is a general Template Method then there's binary compatibility to consider, but I thought it wasn't a general Template Method, just an NVI wrapper. – Hatty

@SteveJessop: but why promote NVI if not to allow more than a shallow wrapper ? pre-cond and post-const are just an example, it could also be maintaining a certain signature for ABI purposes and transforming the arguments etc... NVI is good design because it allows this transformation. However it's only source-code transparent if the method is inlined, while it ABI transparent if it's not. – Benzophenone

I think my point is just that Template Methods in principle do not have to be non-virtual, and non-virtual base class functions in principle do not have to be template methods. The value of an NVI stub that will always be trivial is the last benefit Sutter lists, "The two interfaces are just explicitly separated, is all, and that is a Good Thing". But you're right, I shouldn't have said "it will always be trivial", I should have said, "if you choose to document the interface between Base and its subclasses that way, then you can ensure it will always be trivial". – Hatty

IME, if you document a Template Method that says, "uses FooImpl to do all the work, may do stuff before and after", then you end up going back later to be more specific what kind of stuff. Tracing is safe, enforcing documented pre- and post-conditions is safe. But those are also things that if you miss out on them don't break binary compatibility anyway. Changing the signature of FooImpl breaks source compatibility with subclasses, so it may or may not be worth anything that it's binary compatible with clients, depending where the subclasses come from (your lib, the client, a plugin). – Hatty

@anxieux, I completely agree that one should prefer private virtuals (see my question #3082810), but let's say someone else wrote Base, which I can't change, and I am writing Derived. Or Base::Foo() is pure virtual. – Hoshi

Answer 3

This depends on your design, whether you want to access the virtual function with a derived class object or not.
If not then yes, it's always better to make them private or protected.

There is no code execution difference based on access specifier, but the code becomes cleaner.
Once you have restricted the access of the class's virtual function; the reader of that class can be sure that this is not going to be called with any object or pointer of derived class.

Dolliedolloff