Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add guidelines for NVI idiom #1141

Closed
Urmeli0815 opened this issue Feb 19, 2018 · 6 comments
Closed

Add guidelines for NVI idiom #1141

Urmeli0815 opened this issue Feb 19, 2018 · 6 comments

Comments

@Urmeli0815
Copy link

Urmeli0815 commented Feb 19, 2018

Back in 2001 Herb published a nice article called "Virtuality" in the C/C++ Users Journal which contains four guidelines on how to use access modifiers with virtual methods. These guidelines were:

Guideline #1: Prefer to make interfaces nonvirtual, using Template Method.
Guideline #2: Prefer to make virtual functions private.
Guideline #3: Only if derived classes need to invoke the base implementation of a virtual function, make the virtual function protected.
Guideline #4: A base class destructor should be either public and virtual, or protected and nonvirtual

Guideline #4 is already covered with C.127.

It would be nice to have the other three guidelines integrated, too.

As I work with a code base where public virtual methods are still common sense I experience the negative effects on a daily basis. Maintenance is very difficult: sub-classes call the overridden base-class method anytime (sometimes at the top, sometimes at the bottom and sometimes [and worst] randomly in the middle), replacing the base-class implementation and breaking the LSP, ...

The article is still valid as it is. I would extend it with mentioning the exception of "interfaces" (I.25) which by definition only contain public, pure-virtual methods.

@hsutter
Copy link
Contributor

hsutter commented Feb 19, 2018

I agree we should consider #3.

But we should skip #1 and #2 -- those were considered when we started the Guidelines and kept in the list of candidate rules, but were eventually still considered controversial among the editors and removed. So for now we have decided not to pursue adding them to the Guidelines, though of course libraries can still choose to follow them.

@Urmeli0815
Copy link
Author

Thanks, Herb.

Can you describe where the controversy lies concerning #1 and #2 ? I think it is a very good advice to differentiate between the "interface for the client" and the "interface for the sub-class".

@hsutter
Copy link
Contributor

hsutter commented Feb 19, 2018

In short, public virtual functions are convenient, pandemic(smile), and especially in the case of pure abstract interface types it's often overengineering to separate out "public" vs "virtual" interfaces that would be identical (if we did write up NVI then this would have to be documented as a reasonable exception to the guideline).

@MikeGitb
Copy link

Just for reference: #768

@hsutter
Copy link
Contributor

hsutter commented Feb 26, 2018

On reflection if we don't have #2 then #3 doesn't make much sense on its own (since it's not increasing accessibility). So I think we should defer all of #1/#2/#3 to some point in the future. Also, Gaby points out that module accessibilty decisions could affect our answer in the future.

@AndrewPardoe
Copy link
Contributor

Thanks for the pointer to #768, @MikeGitb, and thanks to @hsutter and @Urmeli0815 for the great discussion. We're going to close this issue without taking any action.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants