Cppcoreguidelines: Add exception to R.11 for code using framework libraries that include memory management

[beojan]

When using some large (framework) libraries like ROOT or Qt, the framework's memory model involves allocating objects with new that are not explicitly deleted later. An explicit exception to R.11 would avoid people employing tricks like std::make_unique(...).release().

Fighting the libraries memory model to use smart pointers will lead to more (and harder to debug) bugs than working with it.

[arvidn]

I think it could be argued that those frameworks should not be using owning raw pointers though.

[beojan]

It doesn't really matter whether they internally use owning raw pointers, that's an implementation detail . The point is that a pointer returned by new isn't necessarily an owning pointer.

Ideally you would be able to use contracts to mark specific constructors (/ sets of arguments to constructors) as only being usable with new or as forbidden from being used with new.

As far as guidelines with current standards, the rule should probably just be 'Avoid using owning raw pointers' (R.3 essentially, though you can't implement a smart pointer without using owning raw pointers, hence the 'avoid').

[jwakely]

It doesn't really matter whether they internally use owning raw pointers, that's an implementation detail .

But they don't just use them internally, they use them in the public API. That's not an implementation detail.

The point is that a pointer returned by new isn't necessarily an owning pointer.

But it is. It's always an owning pointer. Passing it to a function which takes ownership immediately doesn't change the fact that what was returned from new was an owning pointer.

[beojan]

In Qt the constructor takes a pointer to the parent, so the returned pointer is non-owning. If you create an object with no parent, then you have an owning pointer and should use make_unique, remembering to release if you later set a parent (in other words you have to understand what owns what at each point in your application).

In ROOT the parent is implicitly set based on global state (i.e. objects are owned by the current directory) because the fact that the object occupies memory is itself an implementation detail. Again, new returns a non-owning pointer.

[arvidn]

@beojan I don't think it's obvious why that design is a good idea and why it should be endorsed by the guidelines

[beojan]

The design is what it is, and it works. In the case of Qt, it makes sense, since you can call a function to set up your GUI and not have to keep explicit pointers to all the components. In ROOT, the design lets partial / incremental loading from disk be provided seamlessly.

If it isn't possible to follow the guidelines and use these libraries, people will

1. Conclude that the guidelines are not applicable to them, or
2. Find creative tricks (like make_unique().release) sometimes leading to double frees or use after frees,

since it may not be possible or desirable to use a different library.

[neithernut]

The design is what it is, and it works. In the case of Qt, it makes sense, since you can call a function to set up your GUI and not have to keep explicit pointers to all the components.

You'll hardly encounter projects putting together user interfaces together _by hand_ using Qt, at least I hope so.

If it isn't possible to follow the guidelines and use these libraries, people will

Conclude that the guidelines are not applicable to them, or
Find creative tricks (like make_unique().release) sometimes leading to double frees or use after frees,

since it may not be possible or desirable to use a different library.

The Qt people did a pretty good job with coming up with a C++ dialect which is really a language of its own. This becomes apparent when you try to develop a GUI from bare classes: you are literally forced to use their toolchain because they rely on their custom reprocessing. Hence, one could argue that the core guidelines simply do not apply.

Btw: a sane way to have managed objects like this would be to use a factory pattern: have a static function in each class which returns a managed pointer (of the classe's type). Having any management logic in the constructor is just plain insane. It _does_ make things easier for people extending Qt with custom classes through inheritance, but the technical debt it incurs is unacceptable.

Well, the Qt team never constrained themselves to sane designs, anyway. At one point, for example, gcc was at the brink of breaking all Qt-applications by optimizing nullptr-checks on this.

[brenoguim]

The design is what it is, and it works. In the case of Qt, it makes sense, since you can call a function to set up your GUI and not have to keep explicit pointers to all the components.

You'll hardly encounter projects putting together user interfaces together by hand using Qt, at least I hope so.

That statement is far from reality, unfortunately.

[jwakely]

I'm not sure what concrete suggestion there is to improve the guidelines. I don't think explicitly calling out Qt and/or ROOT is a good idea.

R.11 already says: _"If you have a naked new, you probably need a naked delete somewhere, so you probably have a bug."_ If the object created by the new-expression doesn't need an explicit delete, then that statement doesn't apply (note it says _probably_, not definitely).

Doing something silly like make_unique<X>().release() doesn't help anything, it actually hides the Qt-style or ROOT-style dynamic lifetime behind something that looks like normal C++ with a guaranteed leak.

The enforcement rule can't say "unless the code is using Qt or ROOT" because an automated checker can't know that.

R.3 already has a lengthy discussion on the topic of third-party APIs that don't follow the guidelines for using raw pointers as owners.

So the real solution is to engage brain when writing C++, and if you know that your naked new without an accompanying delete is correct, then that's correct.

[ifknot]

Jonathan Wakely provides the correct answer to this.

[beojan]

Concretely, I would add

Exception: When using a library with automatic memory management, use new if the pointer returned is non-owning. If the returned pointer is owning and the code can be refactored to make it non-owning, do so.

Then, the enforcement section can just say check for naked delete.

I agree about engaging your brain, but both libraries are used by people just starting to learn C++ (ROOT in particular, where beginners often have minimal to no C++ experience).

[hsutter]

Editors call: In general, the Guidelines advertise that they are meant for modern code. We agree there will be legacy code that does not follow the Guidelines, and that you need to implicitly treat as exceptions and deal with in whatever way the old code expects. We recommend encapsulating such code behind good interfaces where possible; see P.11.