Skip to content

Conversation

storopoli
Copy link
Contributor

Nothing out of the ordinary.
The most alarming was the non-inclusive range in
test_utils.rs.

In some test functions I allowed myself to use
#[allow(clippy::type_complexity)] because clippy was asking to create a type instead of using the big tuple. Since these were used just once, I thought it was overkill.

Note, depends on:

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Thanks for doing these man, appreciated - and the linter found a bug in the tests, linting for the WIN.

ACK bb29ecc

src/psbt/mod.rs Outdated
.sha256_preimages
.get(&Pk::to_sha256(h))
.and_then(try_vec_as_preimage32)
.and_then(|x: &std::vec::Vec<u8>| try_vec_as_preimage32(x))
Copy link
Member

Choose a reason for hiding this comment

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

This won't work in no-std builds, just use plain old Vec, its imported already in this file using use prelude::*.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, this seems obvious now in retrospect...

let mut pk_map = HashMap::new();
let mut pkh_map = HashMap::new();
for (i, c) in (b'A'..b'Z').enumerate() {
for (i, c) in (b'A'..=b'Z').enumerate() {
Copy link
Member

Choose a reason for hiding this comment

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

This changes the behaviour of the loop to now include 'Z', from looking at the code it looks like this was a bug and the change is correct but this change warranted a patch on its own with a comment on the bug. Since its test code, I think we can leave it as is now though.

BTW I've got deja vu reviewing this, I've definitely removed these lint warnings before somewhere. I can't remember where but I remember hitting this 'Z' thing especially.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I remember this too. #523 It was even mixed in with CI crap.

@tcharding
Copy link
Member

Woops I acked, but needs the Vec change. CI should catch that though.

@apoelstra
Copy link
Member

@storopoli ok, I merged your other two PRs. Can you rebase this one and get CI working?

Nothing out of the ordinary.
The most alarming was the non-inclusive range in
`test_utils.rs`.

In some test functions I allowed myself to use
`#[allow(clippy::type_complexity)]` because clippy
was asking to create a type instead of using the big
tuple. Since these were used just once, I thought it
was overkill.

Note, depends on:

- #639 (we can also merge this into it
instead of `master`)
- #638 (has some allows on dead_code)
@storopoli storopoli requested a review from apoelstra February 17, 2024 16:25
@apoelstra
Copy link
Member

Hooray!

@apoelstra
Copy link
Member

Lol @ the type_complexity one. We probably should actually create a struct for that. But that's a job for another PR.

Thanks very much for fixing CI. Funny how every clippy update finds a new category of "unnecessary &" errors that it previously did not detect.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK e2636f3

@storopoli
Copy link
Contributor Author

Lol @ the type_complexity one. We probably should actually create a struct for that. But that's a job for another PR.

Probably a simple Tuple Struct

Funny how every clippy update finds a new category of "unnecessary &" errors that it previously did not detect.

Yes, clippy lints are truly amazing. Never stops to impress me...

@apoelstra apoelstra merged commit 18b1b30 into rust-bitcoin:master Feb 17, 2024
@storopoli storopoli deleted the js/clippy-lints-1.56.1 branch February 17, 2024 22:17
heap-coder added a commit to heap-coder/rust-miniscript that referenced this pull request Sep 27, 2025
e2636f390a41e43292b47a2eb7aeea1e3aa34a50 Clippy lints MSRV 1.56.1 (Jose Storopoli)

Pull request description:

  Nothing out of the ordinary.
  The most alarming was the non-inclusive range in
  `test_utils.rs`.

  In some test functions I allowed myself to use
  `#[allow(clippy::type_complexity)]` because clippy was asking to create a type instead of using the big tuple. Since these were used just once, I thought it was overkill.

  Note, depends on:

  - #639 (we can also merge this into it instead of `master`)
  - #638 (has some allows on dead_code)

ACKs for top commit:
  apoelstra:
    ACK e2636f390a41e43292b47a2eb7aeea1e3aa34a50

Tree-SHA512: d1f38d1e2a1415da816898806377c90986072fe0bc9a9dbc3e5476026c05c53743453cbd0a237a70aa27762f1be86e44c19422575b7e62f686140accc412177c
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