rule proposal: C.141 Prefer to make virtual functions private.

[cubbimew]

Issue #484 switched the little TODO item on NVI from "NO" to "YES" back in january, but nothing else happened on this subject, as far as I can see. Could this TODO item be addressed?

I had a yet another discussion about it elsewhere, and it occurred to me that there is a parallel with public accessors that helps rationalize the design choices here: classes with all member functions public virtual are like classes with public data members, classes with a non-public virtual do_foo() for every public non-virtual foo() (std::time_get) are like classes with a getter and setter for every data member, and classes where public and customization interfaces are actually different (std::streambuf) are similar to classes with meaningful interfaces that model behavior not structure, tell-don't-ask, and all those good things.

Perhaps an NVI rule could look something like this?

C.141: Prefer to make virtual functions private.

Reason: Separation of concerns: combining customization points and public interfaces creates undue coupling of the user code to class implementation details and encourages unnecessary virtual functions (see C.132: Don't make a function virtual without reason). Public member functions and virtual member functions have different goals and different users.

Example:
bad

class buffer {
 public:
    virtual char get();
    virtual char peek();
    virtual void put(char);
    virtual void bump();
};

good:

class buffer {
 private:
    virtual void overflow();
    virtual void underflow();
 public:
    char get();
    char peek();
    void put(char);
    void bump();
};

Notes:
If derived classes need to invoke the base implementation of a virtual function, make the virtual function protected.
Duplicating each public non-virtual foo() with a non-public virtual do_foo() may indicate poor separation of concerns (see C.132)

Exceptions:
The base class destructor should be either public and virtual, or protected and nonvirtual (see C.127)

Enforcement:
Report public virtual functions other than destructors.
Report non-virtual functions that do nothing but call a virtual function.

...not sure about that enforcement, it may have to be suppressed too often, but on the other hand, it is not really different from avoiding both public data members and "functions that simply access a member" (C.131)

[MikeGitb]

Sounds good, but I don't understand this:

Duplicating each public non-virtual foo() with a non-public virtual do_foo() may indicate poor separation of concerns (see C.132)

Isn't that exactly what we would expect from an interface class that is following the NVI Idiom?

[cubbimew]

Isn't that exactly what we would expect from an interface class that is following the NVI Idiom?

That's what people seem to expect, yes, and I'm proposing that it's as poor practice as providing a dumb accessor for each member -- I don't mind losing that suggestion, but I'd love to gather counterarguments.

[MikeGitb]

Are you saying we should get rid of interface inheritance? Or not use NVI for this purpose? Just want to make sure we are talking about the same thing.

[scraimer]

Either way, anything done under the assumption of design based on NVI should be clearly labeled. Otherwise it would be just as confusing to someone jumping to one of guideline explaining this.

[cubbimew]

Okay, I'd add

Exceptions:
Pure interfaces (see C.121) are composed entirely of public pure virtual functions

[gdr-at-ms]

Why private and not protected? I don't understand. If the derived class can't or shouldn't access the function from the base class, why should it be overriding it? How is chaining modified in this case and why is that a better programming technique?

[galik]

I think we have (at least) two very different use-cases for virtual interfaces to consider.

One is when you want to be able to provide custom or system implementations of a given interface. These are things like std::iostream an co.

Obviously they focus on calling down to root out the specific functionality.

But a completely different use is when you want to provide the client software with a public interface so that they can provide you with objects that conform to your processing requirements. Those have to be all public virtual interfaces.

This distinction may be what C.129 is leaning towards but I'm not sure if it's exactly the same thing.

[gdr-at-ms]

Regarding first case: if it is an implementation of a given interface, that interface has already made it a public function. The only time a virtual function isn't public is when it is to introduce a parameter (fixed by derived classes) -- not an interface or a specification.

[cubbimew]

If the derived class can't or shouldn't access the function from the base class, why should it be overriding it?

If a derived class is overriding a function from the base class, why would it ever have to pretend to be an external user and access that function? Although indeed there are cases where derived classes call into the base versions of the same function they are customizing - that's why protected: is a useful fallback after private virtual. I don't see examples of that in the standard library though (quickly glacing over filebuf and codecvt), although I didn't look at every overrider.

[cubbimew]

Also, if there is no clear consensus on trade-offs and usage guidelines of NVI, perhaps the TODO item can be dropped with no further action? (or perhaps with a small note in C.132)

I find it worrying that there is a list of TODOs from over a year ago in the main document.

[gdr-at-ms]

If a derived class is overriding a function from the base class, why would it ever have to pretend to be an external user and access that function?

That does not explain why private or even why protected is a fallback.

[MikeGitb]

@gdr-at-ms & @cubbimew: Just to make sure we are talking about the same thing:

My understanding of what an NVI Interface looks like is this:

class MyNVInterface {
public:
    int foo(int i){
        //enforce contract, logging ...
        auto res = foo_impl(i);
        //enforce contract, logging, ...
        return res;
    }
private:
    virtual int foo_impl(int)=0;
};

class MyClass : public MyNVInterface {
    int foo_impl(int i) override {
        return i*i;
    }
};

int main() {
    std::vector<MyNVInterface *> classes;
    //...
    for (auto&& e : classes) {
        std::cout << e->foo(10)<<"\n";
    }
}

foo_impl is supposed to be called only from foo, so it should be private by default.

And the general reason to do this instead of the classic way:

class MyInterface {
public:
    virtual int foo(int i) = 0;
};

Is that I can not only enforce a certain syntax (e.g. "there is a function named foo which takes an int and returns an int"), but can also enforce some semantical properties (e.g. "The returned int will always be positive" / "A call to foo will initialize the object if it wasn't initialized before").

So far I've discussed NVI in the context of writing interfaces. A different problem that may nevertheless lead to the same construct is the question of how to provide costumization points in the case of classes which are meant to be inherited from (implementation inheritance).

Here one possiblity is to have virtual functions that are called by the base class and have to / can be overriden by the derived class that is created by the user. A prominent example is QThread, where (at least prior to 4.4?) you where supposed to override the protected run() function, which then would be called from the internals of QThread in the context of a different OS-Thread. Just as before those functions are almost always meant to be called only from within some base class function and hence should be private/protected.

So far my understanding of the two problems touched in this discussion. Is this also what you are talking about or am I completely lost and/or have missunderstood the point of NVI?

[gdr-at-ms]

@MikeGitb I suppose I should make it clear that I am familiar with NVI and I am questioning it as a general sound programming technique that has to be encouraged by the Core Guidelines.

Note also that in your example, the overrider MyClass::foo_impl is public :-)

[hsutter]

FWIW, I find NVI sound but I don't want it to be a distraction so perhaps we should remove it for that reason.

Slightly longer:

Pro: It's been rediscovered multiple times in multiple languages, and last time I looked the standard library de facto follows it closely -- except mainly for std::exception::what(), when I looked pretty much every other virtual function in std:: was nonpublic.

Con: NVI can be abused (e.g., duplicating public and virtual functions) and can definitely be a distraction here. I'm also fine with removing NVI for the reason of its being a distraction, just so we can spend the time on lots of other good stuff instead.

[gdr-at-ms]

If you have a good module system where private members aren't exported, it becomes most confusing to have to override something you can't even see, let alone access.

[BjarneStroustrup]

I do not think we should encourage NVI. Often, the non-virtual function does nothing but forward, doubling the number of member functions and requiring a fancy naming scheme. Like a trivial getter or setter it prepares for a change that never happens.

[AndrewPardoe]

Thanks, Sergey. We will not create a full rule, and you'll remove the TODO as part of #811.