Skip to content

Convert noisy assertions to warnings - #16040

Closed
ywwg wants to merge 1 commit into
mixxxdj:mainfrom
ywwg:owilliams/iteratorfrom
Closed

Convert noisy assertions to warnings#16040
ywwg wants to merge 1 commit into
mixxxdj:mainfrom
ywwg:owilliams/iteratorfrom

Conversation

@ywwg

@ywwg ywwg commented Feb 25, 2026

Copy link
Copy Markdown
Member

We know this can happen, and that it's not fatal to performance, so this can just be a warning.

Part of #13262.

We know this can happen, and that it's not fatal to performance, so this can just be a warning.

Part of mixxxdj#13262.

Signed-off-by: Owen Williams <owilliams@mixxx.org>
@ywwg

ywwg commented Feb 25, 2026

Copy link
Copy Markdown
Member Author

I don't know if this is a good message, or approach

Comment thread src/track/beats.cpp
it = std::lower_bound(cfirstmarker(), clastmarker() + 1, position);
}
DEBUG_ASSERT(it == cbegin() || it == cend() || *it >= position);
DEBUG_ASSERT(it == cbegin() || it == cend() || *it > *std::prev(it));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should keep this assert. If this one was to fail, this would be concerning and a debug assert is probably wise!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sorry, it's hard to tell which one you mean -- the second assert on 471?

Comment thread src/track/beats.cpp
DEBUG_ASSERT(it == cbegin() || it == cend() || *it >= position);
DEBUG_ASSERT(it == cbegin() || it == cend() || *it > *std::prev(it));
if (!(it == cbegin() || it == cend() || *it >= position)) {
qWarning() << "Beats::iteratorFrom: unexpected iterator result, possibly invalid beats";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we put this into a qDebug since we know it has a low impact? Also, in lines with the finding, probably worth to highlight this is likely related to a rounding issue

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

actually I am thinking since we have a good investigation going on the rounding problem, it's not worth changing these asserts at all.

@ywwg ywwg closed this Mar 7, 2026
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

Successfully merging this pull request may close these issues.

2 participants