Improve the examples and guidance on rule C.121 #1608
Conversation
The old example was irrelevant to C.121, but belonged with C.35.
| ##### Example, bad | ||
|
|
||
| class Goof { | ||
| class NVIConnection { // also good |
There was a problem hiding this comment.
maybe keep just one? The protected: section in this one may be distracting.
There was a problem hiding this comment.
Hmm. We could just remove the line protected:, making those members public; that's what I'd do in production code anyway.
I'm loath to show a lot of examples of polymorphic hierarchies in the pre-NVI, Java-flavored style; people might get the idea that the Guidelines are in favor of that style. For this rule in particular I want to clarify that NVI does not conflict with the Guidelines.
There was a problem hiding this comment.
The guidelines are actually not in favor of either. I tried introducing NVI as a rule in #768
There was a problem hiding this comment.
Editors call: Could you please remove the NVI example?
| @@ -6903,43 +6903,57 @@ not using this (over)general interface in favor of a particular interface found | |||
| ##### Reason | |||
|
|
|||
| A class is more stable (less brittle) if it does not contain data. | |||
| Interfaces should normally be composed entirely of public pure virtual functions and a default/empty virtual destructor. | |||
| An interface should be composed entirely of pure virtual functions and a virtual destructor; see [C.35](#Rc-dtor-virtual). | |||
| An interface should have a defaulted default constructor. | |||
There was a problem hiding this comment.
Arthur, can you elaborate on why to add that it should have a defaulted default constructor?
There was a problem hiding this comment.
Editors call: Below it mentions that then derived classes don't need to write explicit constructors, but that's an implementation detail and not part of the basic pattern. Could you remove the part about constructors from this PR? Thanks!
There was a problem hiding this comment.
@hsutter: It sounds like you found the part that answered your question. :) Some people like to =delete the move and copy members of a polymorphic class, just to drive home that polymorphic classes must never be moved or copied. If you =delete any constructor, then you've user-declared that constructor, and therefore you don't get any implicitly declared default constructor — at which point your base class is useless (see https://godbolt.org/z/UjH-MJ ).
"Ah, but what if I also provide a user-defined constructor such as explicit MyBase(int)?" — Well, what purpose does that int serve? It can't be used to initialize any member data, because abstract base classes should not have member data. Therefore the only kind of constructor that makes sense for an abstract base class is a default constructor — and you might as well explicitly default it — and if you delete your copy operations without defaulting your default constructor, you have a bug.
TLDR: Your base class must either =default its default constructor, or not-=delete its copy operations. If you delete your copy operations without defaulting your default constructor, you have a bug.
Personally I would accept "Never =delete a polymorphic type's copy operations (but make sure never to call them either)" as a guideline, but I think realistically that would be super controversial. So I decided to stay silent on the =delete question, but add "always =default your base class's default constructor" so that this guideline would work for both people who do =delete and people (like me) who don't.
(EDIT: oh but also the GMock thing I actually said. Heh.)
The old example was irrelevant to C.121, but belonged with C.35.