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

Remove the std::optional or the std::shared_ptr used beats to prevent potential bugs #13806

Open
acolombier opened this issue Oct 28, 2024 · 4 comments

Comments

@acolombier
Copy link
Member

          This is not understandable without looking up the implementation which is out of sight here. 

auto is here std::optional<BeatsPointer> = std::optional<std::shared_ptr<const Beats>>
std::shared_ptr is already optional. So wrapping it into a std::optional is redundant.
Currently the pointer nature is hidden, which may lead to missing null checks.

With this construct we have to deal with nullptr and nullopt.
In case trySetBeats() returns nullptr, the beats are deleted from the track. I don't think this is the desired behaviour here. So let's just remove the optional wrapper.

Originally posted by @daschuer in #13330 (comment)

@Swiftb0y
Copy link
Member

sometimes (while certainly inefficient) nested optionals are legimitate (see the ColorPalette code), so we need to ensure std::optional(nullptr) and std::nullopt are not treated differently before eliminating the nesting.

@daschuer
Copy link
Member

daschuer commented Oct 29, 2024

In this case tryAdjustTempo() we only have single information transported in the optional state = Successful. Both states he to be treated the same to not remove the old beat grid in the "Unsuccessfull" case in trySetBeats()
The nullptr check is currently missing. @Swiftb0y can you confirm?

@Swiftb0y
Copy link
Member

yes, I can confirm that for tryAdjustTempo(), I have not checked the other member functions with the same return signature yet.

@daschuer
Copy link
Member

Thanks

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

No branches or pull requests

3 participants