-
Notifications
You must be signed in to change notification settings - Fork 3
fix uninitialized variables #1
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
base: main
Are you sure you want to change the base?
Conversation
added const-ness and proper initialization.
| struct State { | ||
| Array real, imag; | ||
| }; | ||
| State newState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be in the class, instead of a stack variable?
geraintluff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I might not understand what this is fixing. Could you give an example of the warning/error you get, or the unexpected behaviour this is preventing?
| // Really we're just doing: state[i] = state[i]*poles[i] + x*coeffs[i] | ||
| // but std::complex is slow without -ffast-math, so we've unwrapped it | ||
| State state = states[channel], newState; | ||
| State& state = states[channel]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is changing this to a reference deliberate?
| } | ||
|
|
||
| Complex operator()(Sample x, int channel=0) { | ||
| std::pair<Sample, Sample> operator()(Sample x, int channel=0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an advantage over Complex, and shouldn't this match the other operator() as well?
| struct HilbertIIRCoeffs { | ||
| static constexpr int order = 12; | ||
| std::array<std::complex<Sample>, order> coeffs{{ | ||
| const std::array<std::complex<Sample>, order> coeffs{{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were these giving warnings in a particular compiler, without const? Or was this for tidiness? 😄
|
Hi @aleksandarkoruga! Just checking in to see if you'd read my comments. I appreciate the PR, but just need to understand it fully before I can do anything with it. |
added const-ness and proper initialization.