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

[Code Quality]: Remove unused #[cfg] attibutes and fix all clippy warnings #358

Open
Davidson-Souza opened this issue Jan 28, 2025 · 6 comments
Labels
code quality Generally improves code readability and maintainability good first issue Good for newcomers

Comments

@Davidson-Souza
Copy link
Collaborator

Clippy gives warnings about unused features, most of them are either related to the cli-blockchain or the slip132 code. The former can be removed altogether, while the latter can be simplified a lot.

@Davidson-Souza Davidson-Souza added code quality Generally improves code readability and maintainability good first issue Good for newcomers labels Jan 28, 2025
@siddheshzz
Copy link
Contributor

Hello @Davidson-Souza , can i give it a try to this.

Upon looking at the code base i found-
For unused #[cfg] attributes, I can see some are disabled by default which can be removed and most of them related to

#[cfg(feature = "metrics")]
#[cfg(not(feature = <FEATURE>))].                           eg - #[cfg(not(feature = "bitcoinconsensus"))]

#[cfg(feature = "cli-blockchain")]
#[error("Json-Rpc error")]
JsonRpcError(#[from] UtreexodError),



#[cfg(feature = "zmq-server")]
#[cfg(target_env = "gnu")]

}

For Clippy warning :

warning: this expression creates a reference which is immediately dereferenced by the compiler
   --> crates/floresta-wire/src/p2p_wire/node.rs:230:44
    |
230 |                 Self::resolve_connect_host(&address, Self::get_port(config.network.into()))
    |                                            ^^^^^^^^ help: change this to: `address`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
    = note: `#[warn(clippy::needless_borrow)]` on by default

warning: `floresta-wire` (lib) generated 1 warning (run `cargo clippy --fix --lib -p floresta-wire` to apply 1 suggestion)

I will give it a try if this sound good.
Thanks

@Davidson-Souza
Copy link
Collaborator Author

Hi @siddheshzz. Thanks for taking a look at this.

Most of those #[cfg] are still useful. bitcoinconsensus, metrics, zmq and json-rpc for example are features we support. Useless mean that we don't have a corresponding feature on the manifest, and clippy will warn about it.

@siddheshzz
Copy link
Contributor

siddheshzz commented Jan 30, 2025

Hi @Davidson-Souza , Thank you for your time.

I am trying to understand few things, please bare with me
On cargo +nightly clippy --all-targets i found few [cfg] warnings - i checked in manifest the feature is not listed
.

  1. In slip132 serde is not explicitly listed under the features section in Cargo.toml but its working fine as an enabled one.
    On reading more on this i found the feature can be implicitly Enabled by another feature(Transitive).
    I tried to add feature serde:["dep:serde"] but this wont work as it will break other Serialization and Deserialization so i imported those.
    I added new feature to cargo.toml making serde and strict_encoding as optional ,enabling serde feature-
serde = { version = "1.0", features = ["derive"], optional = true }
strict_encoding = { version = "1.0", optional = true }
.
.
serde = ["dep:serde"]
default = ["experimental-p2p", "json-rpc", "serde"]

  1. #[cfg(feature = "cli-blockchain")] also came up in warning which was mostly used in Error enum for UtreexodError(UtreexodError),- Removed these #[cfg] and also check all references if any was in use.

Ran cargo test --release - no test breaking/failing

Now the clippy wont show any #[cfg] slip132 & cli-blockchain related warnings.

Is this sound ok or we need some other approach

@Davidson-Souza
Copy link
Collaborator Author

For 1, the code can be massively simplified, and we can assume that serde is always available (we use it for config parsing in florestad)

For 2. This module can be removed as whole, we don't use it any more.

If you want, you can make one pr for each, and another one fixing the last few warnings

@siddheshzz
Copy link
Contributor

We can close this ref- #361 & #367

@Davidson-Souza
Copy link
Collaborator Author

We can close this ref- #361 & #367

I still see some warnings when I run just lint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Generally improves code readability and maintainability good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants