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

Improve the examples and guidance on rule C.121 #1608

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 35 additions & 21 deletions CppCoreGuidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arthur, can you elaborate on why to add that it should have a defaulted default constructor?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@Quuxplusone Quuxplusone May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.)


##### Example

class My_interface {
class ClassicalConnection { // good
public:
// ...only pure virtual functions here ...
virtual ~My_interface() {} // or =default
virtual std::string read() = 0;
virtual void write(std::string_view) = 0;
virtual ~ClassicalConnection() = default;
};

##### Example, bad

class Goof {
class NVIConnection { // also good
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe keep just one? The protected: section in this one may be distracting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guidelines are actually not in favor of either. I tried introducing NVI as a rule in #768

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Editors call: Could you please remove the NVI example?

public:
// ...only pure virtual functions here ...
// no virtual destructor
std::string read() { return do_read(); }
void write(std::string_view sv) { do_write(sv); }
virtual ~NVIConnection() = default;
protected:
static constexpr int default_buffer_size = 1024;
static std::string_view trim(std::string_view);
private:
virtual std::string do_read() = 0;
virtual void do_write(std::string_view) = 0;
};

class Derived : public Goof {
string s;
// ...
class BadConnection { // bad
public:
explicit BadConnection(int i) : connection_state(i) {}
virtual std::string read() { throw std::runtime_error("unimplemented"); }
virtual void write(std::string_view sv) { std::cout << sv << "\n"; }
protected:
int connection_state = 0;
};

void use()
{
unique_ptr<Goof> p {new Derived{"here we go"}};
f(p.get()); // use Derived through the Goof interface
g(p.get()); // use Derived through the Goof interface
} // leak
Any class that derives from `BadConnection` contains a non-static data member `connection_state`,
even if it has no need for that member. `ClassicalConnection` and `NVIConnection` do not impose
any costs on their derived classes. In particular, this means that a `MockConnection` created
with a framework such as GMock will have minimal size.

The `Derived` is `delete`d through its `Goof` interface, so its `string` is leaked.
Give `Goof` a virtual destructor and all is well.
Any class deriving from `BadConnection` will have to define a constructor in order to explicitly
construct its `BadConnection` subobject (which is not default-constructible). In particular, this
means that a `MockConnection` created with a framework such as GMock must define its own constructor,
rather than following the Rule of Zero.

Finally, a class deriving from `BadConnection` might forget to override one of the virtual member functions,
leading to unexpected runtime behavior. `ClassicalConnection` and `NVIConnection` make every virtual
function also pure, so that a derived class cannot accidentally fail to override one of them.

##### Enforcement

* Warn on any class that contains data members and also has an overridable (non-`final`) virtual function that wasn't inherited from a base class.
* Warn on any non-`final` class that contains data members and also has an overridable (non-`final`) virtual function that wasn't inherited from a base class.

### <a name="Rh-separation"></a>C.122: Use abstract classes as interfaces when complete separation of interface and implementation is needed

Expand Down