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

Remove unused async #943

Merged
merged 3 commits into from
Sep 24, 2023
Merged

Conversation

mkeeter
Copy link
Contributor

@mkeeter mkeeter commented Sep 23, 2023

This PR enables the clippy::unused_async lint, then fixes everything it complains about.

There should be no performance or correctness change, because the functions being edited here – by definition – do not contain a yield point.

There's also a drive-by change of adding #[derive(Clone)] to ExtentInfo, since it's POD and this removes some boilerplate.

@@ -1,6 +1,7 @@
// Copyright 2023 Oxide Computer Company
#![cfg_attr(usdt_need_asm, feature(asm))]
#![cfg_attr(all(target_os = "macos", usdt_need_asm_sym), feature(asm_sym))]
#![warn(clippy::unused_async)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of updating all the files directly, should we just put this into the CI script:

local:pflush$ git diff .github/workflows/rust.yml 
diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml
index 65127d7..e278085 100644
--- a/.github/workflows/rust.yml
+++ b/.github/workflows/rust.yml
@@ -45,4 +45,4 @@ jobs:
     - uses: actions/checkout@3df4ab11eba7bda6032a0b82a6bb43b11571feac # v4
     - uses: dtolnay/[email protected]
     - name: Test Libraries
-      run: cargo clippy --all-targets
+      run: cargo clippy --all-targets -- -W clippy::unused_async

Then, we won't forget to add it to other files and async won't sneak back in?

Copy link
Contributor Author

@mkeeter mkeeter Sep 23, 2023

Choose a reason for hiding this comment

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

If I used this strategy, it wouldn't fail locally (either running cargo clippy or through IDE hints).

I've instead moved this to .cargo/config, which enables it for the entire workspace (and should work for CI too). Once rust-lang/cargo#12115 hits stable and we bump the toolchain, we can use that instead!

Copy link
Contributor

Choose a reason for hiding this comment

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

Having a local config file was what I actually wanted, I just did not find that solution in my
limited search.

And, if this runs in CI too, then we don't need to update the .github/workflows/rust.yml file
to include it.

@mkeeter mkeeter force-pushed the remove-unused-async branch from 73d2042 to 369c367 Compare September 24, 2023 13:55
@mkeeter mkeeter merged commit f2c4b12 into oxidecomputer:main Sep 24, 2023
17 checks passed
@mkeeter mkeeter deleted the remove-unused-async branch September 24, 2023 14:54
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