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

AnalyzerWaveformTest refactoring as a foundation for more tests #11488

Merged
merged 3 commits into from
Apr 17, 2023

Conversation

daschuer
Copy link
Member

The next step is to add a test the compares the resulting waveform.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

The canary test is definitely a little weird and would still break if the analyzer would overrun its buffer significantly. Why do we not bounds check instead?

Comment on lines +37 to +45
for (std::size_t i = 0; i < kCanarySize; i++) {
m_canaryBigBuf[i] = kCanaryFloat;
}
for (std::size_t i = kCanarySize; i < kCanarySize + kBigBufSize; i++) {
m_canaryBigBuf[i] = kMagicFloat;
}
for (std::size_t i = kCanarySize + kBigBufSize; i < 2 * kCanarySize + kBigBufSize; i++) {
m_canaryBigBuf[i] = kCanaryFloat;
}
Copy link
Member

Choose a reason for hiding this comment

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

couldn't these be memsets?

Copy link
Member Author

Choose a reason for hiding this comment

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

memset() works with char arrays only. We can do a lot more refactoring here, but I like to stop at this point.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I forgot about that.

@daschuer
Copy link
Member Author

The canary test is definitely a little weird and would still break if the analyzer would overrun its buffer significantly. Why do we not bounds check instead?

The whole test is kind of pointless, because the buffer is passed to the analyzer as const. So there must be went a lot of things wrong to make this test fail. Maybe it was relevant for an early version of the analyzer? We may consider to remove it.

@Swiftb0y Swiftb0y merged commit 38a962f into mixxxdj:2.3 Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants