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 compiler warnings #4026

Merged
merged 14 commits into from
Feb 28, 2025
Merged

Fix compiler warnings #4026

merged 14 commits into from
Feb 28, 2025

Conversation

har7an
Copy link
Contributor

@har7an har7an commented Feb 26, 2025

After the recent rust toolchain upgrade, we started getting a bunch of warnings during compilations. It turns out they were mostly harmless, but still not pretty to look at.

This PR removes the current compiler warnings and makes cargo x build run warning-free. It does remove a bit of code to get there though. As of right now I've still got an enum in the Client implementation to weed through, where apparently we're not using internal fields in certain variants. I'm currently investigating whether these enum variants are still needed in the first place.

@imsnif What's your stance on deleting "unused" code? In particular the functions handle_right_mouse_release and handle_middle_mouse_release are only used in tests, for example. We're only handling mouse hold/release for left clicks explicitly. Would you prefer I remove that code entirely, or mark it as #[cfg(test)] so it sticks around in case we need it in the future?

@imsnif
Copy link
Member

imsnif commented Feb 26, 2025

Things are still not settled in that area... could we remove the warning?

for xtask subcommand to remove warnings about unknown compiler
attributes.
as dependency, since the code we need can now be expressed using rusts
`std` builtin types.
so the compiler doesn't complain about dead code any more.
across the codebase, as discovered by the rust compiler.
from `ClientInstruction` and all code attached to it.
@imsnif
Copy link
Member

imsnif commented Feb 26, 2025

Actually - I think these unused warnings were introduced by my recent changes with AnyEvent and are not something that comes from new rules in the new rust version, right? If so you can just leave them. I'll deal with them when I finalize stuff before the release.

@har7an
Copy link
Contributor Author

har7an commented Feb 26, 2025

Sorry for the noise, but changes in main seemed to add a few things I hadn't caught before.

Actually - I think these unused warnings were introduced by my recent changes with AnyEvent and are not something that comes from new rules in the new rust version, right?

I suspect so, but I can't prove it without digging deeper.

In any case, they're all gone now. I saw in your commit message for AnyEvent that you're aware that mouse button releasing is currently unfinished? Assuming that is still the case, I marked the handle_*_mouse_release functions as #[cfg(test)], so the tests still pass and you don't have to restore the code from the git history.

There's still the mystery around the CliPipe enum variant thingies I'm not quite sure of. It appears to do nothing much on the client side, but removing the code (and all of its appendages) entirely broke a lot of other stuff. So I followed the compilers recommendation and stubbed the Strings with (). Afaict, these enum fields weren't ever read (not even in tests?), and CI at least says there's no harm done here. I'll keep the PR around until tomorrow and see if I can figure something out about that enum, but other than that this is done IMO.

@har7an har7an merged commit 9f900a7 into zellij-org:main Feb 28, 2025
6 checks passed
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