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 use of parented_ptr #13411

Merged
merged 8 commits into from
Jun 28, 2024
Merged

Conversation

daschuer
Copy link
Member

This PR fixed some cases where a patented_ptr is constructed without a parent. This creates a pointer that is temporary without any owner.

I have moved the assertion to the constructor to avoid this situation in future.

The risk is that we with debug assertion enabled, Mixxx will crash in case I have missed a case. So a double check during review is required.

This is part of the pending discussion about the future of parented_ptr in: #13395

@Swiftb0y
Copy link
Member

Swiftb0y commented Jun 27, 2024

I have moved the assertion to the constructor to avoid this situation in future.

Wdyt about adding some compile time checks to make_parented? This code will fail if make_parented is called without any QObjects (if concepts don't work in all the compilers, you can also replace it with constexpr bool and static_assert). I know its a bit liberal in the sense that it can't know whether the passed object is actually the parent, but it can know that the code is definitely wrong if none of the parameters is a QObject.

template<typename... Args>
concept AnyQObject = (... || std::is_base_of_v<QObject, Args>);

template<typename T, typename... Args> 
parented_ptr<T> make_parented(Args...) requires (AnyQObject<Args...>)

@Swiftb0y
Copy link
Member

whoops. sorry its

template<typename... Args>
constexpr bool AnyQObject = (... || std::is_base_of_v<QObject, std::remove_pointer_t<Args>>);

src/util/parented_ptr.h Outdated Show resolved Hide resolved
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

if you prefer I can also fix it locally and issue a PR to your branch once I got it working.

src/util/parented_ptr.h Outdated Show resolved Hide resolved
@daschuer daschuer force-pushed the improve_parented_ptr branch from 2131409 to 5bb0daf Compare June 27, 2024 17:39
@Swiftb0y
Copy link
Member

ah nice, the static check actually caught something

daschuer added 2 commits June 27, 2024 23:50
… allow the weak situation where an object is not memory managed. This reverts df77064 partially.
@daschuer daschuer force-pushed the improve_parented_ptr branch from 2a5eb7f to 11d2f59 Compare June 27, 2024 21:50
@daschuer daschuer force-pushed the improve_parented_ptr branch from 11d2f59 to 30e9db2 Compare June 27, 2024 21:55
@daschuer
Copy link
Member Author

All green finally. Thank you for your perfect idea with the compile time check.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

LGTM. thank you.

@Swiftb0y Swiftb0y merged commit 15b4444 into mixxxdj:main Jun 28, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants