Remove async feature; misc test cleanup #650
Conversation
✅ Deploy Preview for aya-rs-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1, 2 of 2 files at r2, 5 of 5 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @tamird)
tamird
force-pushed
the
test-cleanup
branch
6 times, most recently
from
35f978f
to
4a252ce
Compare
tamird
force-pushed
the
test-cleanup
branch
15 times, most recently
from
737eff6
to
3b81db6
Compare
| @@ -98,16 +98,17 @@ impl<T: BorrowMut<MapData> + Borrow<MapData>> AsyncPerfEventArray<T> { | |||
| index: u32, | |||
| page_count: Option<usize>, | |||
| ) -> Result<AsyncPerfEventArrayBuffer<T>, PerfBufferError> { | |||
| let buf = self.perf_map.open(index, page_count)?; | |||
| let Self { perf_map } = self; | |||
There was a problem hiding this comment.
you love a destructuring don't you 😅 In this case, how is it better than just doing self.perf_map? It's the only field used and it's used only once
There was a problem hiding this comment.
I sure do! It's just an assertion that no fields have been added and don't need to be dealt with. It's a bit of paranoia that protects this code from future changes that affect semantics.
| let fd = buf.as_raw_fd(); | ||
| Ok(AsyncPerfEventArrayBuffer { | ||
| buf, | ||
|
|
||
| #[cfg(feature = "async_tokio")] | ||
| async_fd: AsyncFd::new(fd)?, | ||
| async_tokio_fd: AsyncFd::new(fd)?, |
There was a problem hiding this comment.
I'm not sure I understand - is the purpose of this change to make cargo hack
happy? This code (pre PR) is messy and I'm glad it's getting improved, but now
it'll let you compile with both features on but won't do the right thing at
runtime. I think we should compile_error or something if both features are on?
Also I'm not sure that merging the read_events implementations is an
improvement. The code seems harder to read and understand to me. Not having to
write docs twice is an improvement, but that can probably be achieved in some
other way.
There was a problem hiding this comment.
We can compile error, but we're in somewhat unsupported territory because mutually exclusive features aren't really a thing in cargo.
We can also do something approximating the old code: pick tokio If both are enabled.
A third option is to crash at runtime. Not clear to me what's best here.
There was a problem hiding this comment.
I think we should keep the current behaviour and use tokio when both are enabled https://doc.rust-lang.org/cargo/reference/features.html#mutually-exclusive-features
This feature is equivalent to async_tokio || async_std; removing it avoids warnings emitted during `cargo hack check --feature-powerset` where async is selected without either of the other features. Use cargo hack to ensure clippy runs on the powerset of features.
This can cause test pollution. Create a new temp directory on each run.
Several tests were not running due to being omitted from this list.
See individual commits.