mixxxdj / mixxx

Mixxx is Free DJ software that gives you everything you need to perform live mixes.
http://mixxx.org
Other
4.5k stars 1.28k forks source link

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

Open acolombier opened 1 week ago

acolombier commented 1 week ago
          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 https://github.com/mixxxdj/mixxx/pull/13330#discussion_r1818186791_

Swiftb0y commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

daschuer commented 1 week ago

Thanks