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

Fix non-linux compilation warnings. #248

Merged
merged 10 commits into from
Jan 31, 2025
Merged

Conversation

tomusdrw
Copy link
Contributor

@tomusdrw tomusdrw commented Jan 7, 2025

I've started adding #[cfg(target_os = "linux")] but for some cases it starts a cascade of related code not being used, so I think it's safer to go with conditionally allowing dead code.

Let me know what's your preference.

@koute
Copy link
Collaborator

koute commented Jan 8, 2025

Hmmm... well, I'm not sure whether gating those to Linux is the way we want to go, due to a few reasons:

  • it's not strictly correct (e.g. on Aarch64 Linux systems you'll also get extra warnings)
  • we will want to support the recompiler on other OSes in the future too (And in fact the recompiler used to work on macOS too in the past, before I had to refactor a ton of code and didn't have the time to get the generic sandbox updated too.)
  • and it seems kinda painful to maintain

So I think what we could do:

  • in cases where the code in question is, not sure how to call it, "general purpose", we could unconditionally allow dead_code (for example, Mutex::lock should never give out a warning even when unused, because the whole point of a mutex is locking, so it doesn't make sense for it to not have this method)
  • for compiler-specific code wrap it in if_compiler_is_supported! { (which is the proper way to gate code); unfortunately this is sometimes somewhat awkward to do (could be made easier with a procedural macro, but I refuse to bloat up the compile times for everyone just so that it's a little bit more convenient for me)
  • Run clippy on the CI twice, but use a target which doesn't have a recompiler the second time to make sure the warnings don't come back

@tomusdrw
Copy link
Contributor Author

tomusdrw commented Jan 10, 2025

Thanks for the suggestions! I've left a few allow(dead_code) where I felt it's general purpose. The rest was converted to use if_compiler_is_supported.
I've also added a clippy run on macos to prevent new warnings.

@tomusdrw
Copy link
Contributor Author

Apologies for the borked merge. I run the CI on my fork: FluffyLabs#1 and it passes, so it should be ready for re-review.

@tomusdrw tomusdrw requested a review from koute January 20, 2025 19:56
@koute koute merged commit 19d4805 into paritytech:master Jan 31, 2025
12 checks passed
@koute
Copy link
Collaborator

koute commented Jan 31, 2025

Thanks!

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