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 the compile error while running the cargo clippy #540

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

git-hulk
Copy link
Contributor

I ran the instruction cargo clippy --all --all-targets -- -D clippy::all according to the README and got below error:

❯ cargo clippy --all --all-targets -- -D clippy::all
    Checking harness v0.1.0 (/Users/hulk/code/rust/raft-rs/harness)
error: you seem to use `.enumerate()` and immediately discard the index
    --> harness/tests/integration_cases/test_raft.rs:5304:54
     |
5304 |     for (_, &(l1, l2, l3, p1, p2, p3, id, state)) in tests.iter().enumerate() {
     |                                                      ^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unused_enumerate_index
     = note: `-D clippy::unused-enumerate-index` implied by `-D clippy::all`
     = help: to override `-D clippy::all` add `#[allow(clippy::unused_enumerate_index)]`
help: remove the `.enumerate()` call
     |
5304 |     for &(l1, l2, l3, p1, p2, p3, id, state) in tests.iter() {
     |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~    ~~~~~~~~~~~~

Signed-off-by: git-hulk <hulk.website@gmail.com>
@git-hulk git-hulk force-pushed the fix-compile-error branch from 59cea58 to 0399217 Compare March 27, 2024 12:31
Copy link
Contributor

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Good to go.

BTW, what's Rust toolchain do you use? Perhaps we can add a rust-toolchain file or add a package.rust-version field in Cargo.toml.

@tisonkun
Copy link
Contributor

@git-hulk more violations are found. Please try to fix them.

@git-hulk
Copy link
Contributor Author

@tisonkun Thanks for your quick review

BTW, what's Rust toolchain do you use? Perhaps we can add a rust-toolchain file or add a package.rust-version field in Cargo.toml.

I'm using the version v1.75.0

@git-hulk more violations are found. Please try to fix them.

sure, will take a look.

@tisonkun tisonkun enabled auto-merge (squash) March 27, 2024 14:50
@tisonkun
Copy link
Contributor

More error ... Weird. Why they are not spotted before in CI.

@tisonkun
Copy link
Contributor

But all about styles / format

@git-hulk
Copy link
Contributor Author

@tisonkun Yeah, I'm fixing them, it might be caused by the rules in the new version of the toolchain.

Signed-off-by: git-hulk <hulk.website@gmail.com>
auto-merge was automatically disabled March 27, 2024 15:13

Head branch was pushed to by a user without write access

@git-hulk git-hulk force-pushed the fix-compile-error branch from 1e2861d to 9be2cfc Compare March 27, 2024 15:13
@git-hulk
Copy link
Contributor Author

@tisonkun I have tested on my side and it works now: https://github.com/git-hulk/raft-rs/actions/runs/8454044888/job/23158169847

@tisonkun tisonkun merged commit 91cd291 into tikv:master Mar 27, 2024
5 checks passed
trait AssertSend: Send {}

impl<T: Storage + Send> AssertSend for Raft<T> {}
impl<T: Storage + Send> Raft<T> {}
Copy link
Member

Choose a reason for hiding this comment

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

This is a wrong change. AssertSend is used to ensure Raft<T> is always Send.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fixing this. Sorry to miss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for wrongly removing this constraint.

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.

3 participants