Refactor(parented_ptr): make trivially destructible in release mode, delete move operations

[Swiftb0y]

not ready yet, just did this for fun and an experiment. wdyt about this? It's obviously not super necessary but it may take of even more burden of a reviewer when they see a make_parented, knowing that it will only work if the object is actually capable of participating in the Object Tree memory management and not just taking a QObject by chance.
Thoughts and reviews welcome even though this is just a draft.

[daschuer]

I did not understand the std::is_invocable_r_v constrains.

[Swiftb0y]

They currently don't work either, but the idea was to confirm that there exists a constructor for the given args. Ideally those args could also be checked to contain at least one QObject (the parent argument), but I didn't figure out how to check that in a reasonable and performant fashion. I wanted to avoid the case where people would use make_parented on a class that supports it, but then don't actually pass a parent object, which would create a leak.

[daschuer]

The parented pointer already checks if the object has a parent. Constructing the object without a parent already fails.

[Swiftb0y]

yes, it does in a debug assert, but for one, thats based on a ducktyping which might result in false negatives (can be solved via static_assert with std::derived_from) and one the other hand it's at runtime even though a large subset of cases could be detected at compile time. Also, the runtime assertion in the destructor also doesn't protect against the problem when qobjects are destructed out-of-order.

[daschuer]

Do you want to replace the runtime assertion?

[Swiftb0y]

No the runtime assertion is fine (mostly, commit is in the works). But the runtime assertion only happens in debug builds on shutdown and is probably only noticed with —DDEBUG_ASSERTIONS_FATAL. I want to turn it into a compile time assertion so its easier to spot.

[daschuer]

How to detect that the parent pointer is not null at compile time? A not_null pointer wrapped does not help, because it is also a runtime check. Maybe with link time assets #4139 or https://github.com/daschuer/mixxx/tree/not_null_link_assert

[Swiftb0y]

How to detect that the parent pointer is not null at compile time?

you can't reliably. Again, I don't want to remove the runtime time check, I just want to catch a subset of cases earlier.

I have reservations against link-time asserts so I don't want to go down that route now.

[Swiftb0y]

got sick of all the compile errors on incomplete compilers. I'll try again in a year or two when we hopefully have clang 15 on macos...

So this PR just degenerated to the "misc improvements" part of the title...

[Swiftb0y]

friendly ping, requesting review.

[daschuer]

for my understanding '~parented_ptr() = default' and ~parented_ptr() {} is the same. So I can imagine that this is not needed.

[Swiftb0y]

No its not equivalent. One if an empty definition which means that its still user provided even if it does nothing, the other one is the default declared one which does not count as user-provided.

See
https://en.cppreference.com/w/cpp/language/destructor#Trivial_destructor
https://compiler-explorer.com/z/cbG99jbzr

[daschuer]

I have never understood why it should be not lowed to copy or move a parented_ptr()
A copy of the parented pointer can be safely moved around since it is not owning the pointer. It is just telling the programmer: "This is a borrowed pointer owned by the parent object".

[daschuer]

Ok, It makes sense to have only one instance checking the ownership. The deleted copy constructor enforces that.
Unfortunately it prevents also to copy the object that contains it.
A move will move the responsibility for checking the parent along with the pointer. There is nothing wrong with it. Is it?
What is your opinion here?

[Swiftb0y]

I didn't really check to be honest. I just saw that we didn't follow the rule of five and thus I chose the safe default. We can lift the restrictions if we can be sure that its safe.

[Swiftb0y]

I have reservations about moving. If we move an object that is owned by its parent, its only safe if the parent knows that it has been moved, do QObjects take care of telling their parent?

[daschuer]

Yes, it is probably OK, we can change it whenever we find a valid use case for it.

[daschuer]

can be solved via static_assert with std::derived_from

this was a good idea IMHO, why did you discard it?

[Swiftb0y]

Because its not supported on the outdated apple-clang we're stuck with and working around that resulted in too much of a hassle and IIRC also resulted in false positives (?)

[daschuer]

LGTM, Thank you.

[Swiftb0y]

Thank you