Add FIXMEs for use of bpm.value() without checking#4165
Conversation
| bool tryGetBeatgrid(BeatsPointer pBeats, | ||
| mixxx::audio::FramePos cuePlayPos, | ||
| int64_t frameCount, | ||
| std::vector<djinterop::beatgrid_marker>* pBeatgrid) { |
There was a problem hiding this comment.
The code in this method is essentially unchanged from before, but has been lifted out of the main exportMetadata() function, and early-exit conditions added if any positions turn out to be invalid.
| qWarning() << "Hot cue" << hotCueIndex << "exists but is invalid for track" | ||
| << pTrack->getId() << "(" << pTrack->getFileInfo().fileName() << ")"; |
There was a problem hiding this comment.
Making the assumption that it is a warning-worthy situation to have a cue point for a track, but for that cue point to have an invalid position.
Pull Request Test Coverage Report for Build 1085927887
💛 - Coveralls |
| bool ok; | ||
| const auto bpmValue = rawValue.toDouble(&ok); | ||
| // FIXME: calling bpm.value() without checking bpm.isValid() | ||
| return ok ? bpmValue : mixxx::Bpm().value(); |
There was a problem hiding this comment.
mixxx::Bpm().value() should be replaced with mixxx::Bpm::kValueUndefined wherever it occurs
|
@Holzhaus Maybe add |
uklotzde
left a comment
uklotzde
left a comment
There was a problem hiding this comment.
Thank you! We can replace the FIXMEs in subsequent PRs.
|
|
|
Huh??? I merged #4058 and somehow GitHub merged this too????? |
|
Strange. I pressed merge once here before I noticed the conflicts and GitHub refused to merge. |
4 participants
This PR does two things:
FIXMEstatements any instances in the code where I could see a call toBpm::value()without an appropriate check or assertion onBpm::isValid()first.With regard to the FIXMEs, it may be the case that some parts of the code I highlighted in this PR are actually safe - but the point of this PR is simply to highlight where any such safety doesn't seem immediately "obvious".
Happy to review any of the FIXMEs (i.e. remove or fix them if trivial!) before this PR is approved.