changed BUFFER_SIZE to usize (cf. #49) #52
Conversation
What do you think @orottier ? |
|
Yes and what is casted here is a constant. Anyway I could understand you don't consider this clean (and the buffer size might also become configurable in the next version of the API), so let me know. Also, I don't really understand why the build failed (using |
Yes, you're right. Another way would be to make a |
| assert_float_eq!( | ||
| output.channel_data(0).as_slice(), | ||
| &ref_sine.data[..], | ||
| ulps_all <= 40 | ||
| ulps_all <= 64 | ||
| ); | ||
| assert_float_eq!( | ||
| output.channel_data(1).as_slice(), | ||
| &ref_sine.data[..], | ||
| ulps_all <= 40 | ||
| ulps_all <= 64 |
There was a problem hiding this comment.
Suggestions:
- Maybe we should switch to absolute tolerance comparison. ULP are difficult to reason about, and since we are using values from a small interval, absolute error should be fine.
- Cherry pick this commit to make a separate PR
There was a problem hiding this comment.
This is something I've noticed too. Sometimes tests on my machine fail with the current ulps_all margins.
I agree we should probably use absolute values for all integration/snapshot tests
tests/offline.rs
Outdated
| // compute modulo before to avoid clippy::assertions-on-constants errors | ||
| let modulo = LENGTH % BUFFER_SIZE; | ||
| assert!(modulo != 0); |
There was a problem hiding this comment.
Suggestions:
- maybe try
assert_ne!(LENGTH % BUFFER_SIZE,0)to see if it passes clippy warning
src/lib.rs
Outdated
| /// Render quantum size (audio graph is rendered in blocks of this size) | ||
| pub const BUFFER_SIZE: usize = 128; | ||
| pub const BUFFER_SIZE: usize = BUFFER_SIZE_U32 as usize; |
There was a problem hiding this comment.
Suggestions:
- replace
BUFFER_SIZEwithBUFFER_SIZE_USIZEto make explicit that there is severalBUFFER_SIZE_*variables, even whenusizetype is used.
There was a problem hiding this comment.
My (not really opinionated) rationale here was that as BUFFER_SIZE_U32 is finally only used in low-level io.rs, maybe it's good to reinforce that it is a kind of "special" const compared to BUFFER_SIZE (which should be preferred).
There was a problem hiding this comment.
I also prefer BUFFER_SIZE as a succinct name, conforming nicely to the spec naming.
If the U32 variant is only used in io.rs, let's move the const there so it will not pollute the pub namespace.
You could even use the usize variant in that file, and only cast at the final moment when passsing it to cpal. In that case we don't even need a const
There was a problem hiding this comment.
ok, used u32::try_from(u: usize) at last moment and clippy doesn't complain
|
Thanks for your work @b-ma, looking very good! |
|
Regarding clippy, I had a couple of errors before getting to the one reported by the CI. It's fixed in 76a1e43. |
|
Excellent. Code is much cleaner now |
Not really sure about these clippy directives, maybe the casting from
usizetou32should be handled in another way (even if I can't see how this could fail):and
(Besides, I have two tests failling (
node::oscillator::tests::default_sine_rendering_should_match_snapshotandnode::iir_filter::test::default_periodic_wave_rendering_should_match_snapshot), I don't think they are related to my changes because they are also failing on the master branch, it seems that the allowed error is too low. I can fill a new issue with more details if it's not something you are already aware of.)