More cue hint for caching reader #4771
Conversation
This allows to add a more sophisticated priority handling
| @@ -70,6 +70,21 @@ inline int hotcueNumberToHotcueIndex(int hotcueNumber) { | |||
| } | |||
| } | |||
|
|
|||
| void appendCueHint(HintVector* pHintList, const mixxx::audio::FramePos& frame, Hint::Type type) { | |||
There was a problem hiding this comment.
we should rethink our "mutable references should be pointers" rule. Taking a value that could be nullptr and assuming its not is simply bad. Especially when the consequence of that assumption is a crash.
| void appendCueHint(HintVector* pHintList, const mixxx::audio::FramePos& frame, Hint::Type type) { | |
| void appendCueHint(HintVector& pHintList, const mixxx::audio::FramePos& frame, Hint::Type type) { |
There was a problem hiding this comment.
This ancient rule causes lots of boilerplate extra code that might contain bugs. I have argued for years until giving up and moving on.
There was a problem hiding this comment.
Yes. It seems this "style-preference" (though IMO its much more than just a matter of style) is in the minority in the C++ community. Even ignoring the Google/Qt vs CppCoreGuidelines "controversy", the simple argument of type-safety (and the thus gained safety) suffices for a decision. The less "illegal" states your type has, the less you need to check before using it. With my Rust experience, whenever I see a pointer in C++, I'm thinking that's an intentional Option<Box<T>> so there is either something special going on with the actual presence of the contained value or its lifetime. Especially because its already trivial to express mutable "borrowing" in C++ by just using a mutable reference.
There was a problem hiding this comment.
The Original google styleguide says:
References can be confusing, as they have value syntax but pointer semantics.
I have never even noticed this being an issue personally. I've struggled to find any arguments on this online, so I invite everyone to come up with their own example because I can neither think of nor find one.
There was a problem hiding this comment.
The null check is now in place.
There was a problem hiding this comment.
But its not needed if one just used a mutable reference. And its not free either...
There was a problem hiding this comment.
Yes, If you assume that a reference cannot be null which is not the case. But anyway, that the Mixxx style, no need to discuss it here.
There was a problem hiding this comment.
Would you mind if we continued this discussion on Zulip? It seems like the majority of mixxx devs is dissatisfied by this style and we'd like to transition away from it.
|
Done. |
|
All green now. |
This is a regression since mixxxdj#4771
This adds the intro outro and sound start cues the the hints for the caching reader.
They are like the hotcues also reachable by a single button that should not cause a cache miss.
This also replaces the unused priority by a type value. That can be used for debugging and a central priority management.
It will also help to implement an decoder offset test using the sound start position.