Skip to content

parented_ptr: Assert parent at destruction instead of construction#2172

Merged
uklotzde merged 7 commits intomixxxdj:masterfrom
haslersn:enhancement/parented_ptr-warn-on-destruct
Jun 17, 2019
Merged

parented_ptr: Assert parent at destruction instead of construction#2172
uklotzde merged 7 commits intomixxxdj:masterfrom
haslersn:enhancement/parented_ptr-warn-on-destruct

Conversation

@haslersn
Copy link
Copy Markdown
Contributor

Previously, parented_ptr asserted on construction that the pointed-to object has a parent. However, sometimes one wants to create an object and assign it to a parent later. The recommendation was to create a unique_ptr p instead, later assign the object to a parent and then create a new variable using make_parented(p.release()).

This has however several drawbacks:

  • There is a short period where the object is already assigned to a parent but still owned by a unique_ptr. This can lead to a double free.
  • There are two pointers that and it depends on the code location which one is the correct one to access the object.
  • It is inconvenient to use, because one has to write an extra line where the unique_ptr is released.

With this commit, parented_ptr asserts that the pointed-to object has a parent at the destruction of the parented_ptr. That means, that one can store the object using a parented_ptr a priori and there will be a warning (from the failing DEBUG_ASSERT) in case any object is leaked.

Optionally, in a future PR, this behaviour can even be changed so as to delete the object that is going to be leaked. That is, the parented_ptr destructor wouldn't only DEBUG_ASSERT that the pointed-to object has a parent, but it would also delete it otherwise.

Additional changes:

  • Made parented_ptr implicitly convertible to a raw pointer. The reasons why the standard library types are not convertible to a raw pointer don't apply to parented_ptr. Regarding the insufficient usage of parented_ptr in Mixxx at the moment, making it implicitly convertible will make its adoption easier because less code has to be changed. (No .get() everywhere.)
  • Added a constructor taking nullptr_t.
  • Added noexcept where applicable.
  • Formatted the code using clang-format.

Previously, `parented_ptr` asserted on construction that the pointed-to
object has a parent. However, sometimes one wants to create an object
and assign it to a parent later. The
[recommendation](https://www.mixxx.org/wiki/doku.php/coding_guidelines#pointer_object_lifetime_ownership)
was to create a `unique_ptr` `p` instead, later assign the object to a
parent and then create a new variable using `make_parented(p.release())`.

This has however several drawbacks:

* There is a short period where the object is already assigned to a
  parent but still owned by a `unique_ptr`. This can lead to a double
  free.
* There are two pointers that and it depends on the code location which
  one is the correct one to access the object.
* It is inconvenient to use, because one has to write an extra line
  where the `unique_ptr` is released.

With this commit, `parented_ptr` asserts that the pointed-to object has
a parent at the **destruction** of the `parented_ptr`. That means, that
one can store the object using a `parented_ptr` a priori and there will
be a warning (from the failing `DEBUG_ASSERT`) in case any object is
leaked.

Optionally, in a future PR, this behaviour can even be changed so as to
delete the object that is going to be leaked. That is, the
`parented_ptr` destructor wouldn't only `DEBUG_ASSERT` that the
pointed-to object has a parent, but it would also delete it otherwise.

Additional changes:

* Made `parented_ptr` implicitly convertible to a raw pointer. The
  reasons why the standard library types are not convertible to a
  raw pointer don't apply to `parented_ptr`. Regarding the insufficient
  usage of `parented_ptr` in Mixxx at the moment, making it implicitly
  convertible will make its adoption easier because less code has to
  be changed. (No `.get()` everywhere.)
* Added a constructor taking `nullptr_t`.
* Added `noexcept` where applicable.
* Formatted the code using `clang-format`.
Comment thread src/util/parented_ptr.h Outdated
Comment thread src/util/parented_ptr.h
Comment thread src/util/parented_ptr.h Outdated
Comment thread src/util/parented_ptr.h Outdated
@uklotzde
Copy link
Copy Markdown
Contributor

@haslersn Would you please sign our contributor agreement that allows us to publish your code:
https://docs.google.com/forms/d/e/1FAIpQLScC9QG327sjLL0eWftmfGUasxFFLxg6LCXJ2xHDYRzFIRqyiw/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ

I confused you with another first-time contributor who provided the NixOS files a few days ago.

Comment thread src/util/parented_ptr.h
public:
parented_ptr() noexcept = default;
parented_ptr() noexcept
: m_ptr{nullptr} {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch, I didn't notice either.

@uklotzde
Copy link
Copy Markdown
Contributor

LGTM. Thank you for improving the overall code quality and for digging deep to fix edge cases!

@uklotzde uklotzde merged commit 4a479f2 into mixxxdj:master Jun 17, 2019
@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Jun 17, 2019

I'm getting build errors now with Clang 8.0.0 on Fedora 30:

src/util/parented_ptr.h:23:28: error: expected ';' at end of declaration list
    parented_ptr(nullptr_t) noexcept
                           ^
                           ;

...

src/util/parented_ptr.h:60:29: error: unknown type name 'nullptr_t'; did you mean 'std::nullptr_t'?
    parented_ptr& operator=(nullptr_t) noexcept {
                            ^~~~~~~~~
                            std::nullptr_t
/usr/bin/../lib/gcc/x86_64-redhat-linux/9/../../../../include/c++/9/x86_64-redhat-linux/bits/c++config.h:2328:29: note: 
      'std::nullptr_t' declared here
  typedef decltype(nullptr)     nullptr_t;
                                ^
In file included from src/main.cpp:26:
In file included from src/mixxx.h:30:
In file included from src/util/timer.h:9:
src/util/parented_ptr.h:23:18: error: field has incomplete type 'parented_ptr<ControlProxy>'
    parented_ptr(nullptr_t) noexcept
                 ^
src/util/timer.h:132:32: note: in instantiation of template class 'parented_ptr<ControlProxy>' requested here
    parented_ptr<ControlProxy> m_pGuiTick;
                               ^
src/util/parented_ptr.h:17:7: note: definition of 'parented_ptr<ControlProxy>' is not complete until the closing '}'
class parented_ptr {
      ^

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

Successfully merging this pull request may close these issues.

3 participants