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

feat: remove zeroed vector by pushing front #3227

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

ChrisCho-H
Copy link
Contributor

@ChrisCho-H ChrisCho-H commented Aug 23, 2024

Remove useless zeroed vector following #3131.

  1. Init vector with capacity not to realloc when pushing.
  2. If vector is empty(the first iteration), push data from the front.
  3. Iterate vector from the front, and copy it from the end.

@coveralls
Copy link

coveralls commented Aug 23, 2024

Pull Request Test Coverage Report for Build 10559092245

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • 406 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+0.01%) to 82.758%

Files with Coverage Reduction New Missed Lines %
hashes/src/siphash24.rs 4 97.89%
bitcoin/src/psbt/serialize.rs 6 96.21%
units/src/fee_rate.rs 13 82.02%
hashes/src/internal_macros.rs 16 65.17%
io/src/lib.rs 23 69.39%
bitcoin/src/blockdata/script/owned.rs 31 68.33%
bitcoin/src/crypto/ecdsa.rs 44 46.73%
io/src/bridge.rs 50 0.0%
bitcoin/src/blockdata/transaction.rs 91 89.7%
units/src/amount.rs 128 86.56%
Totals Coverage Status
Change from base Build 10527005040: 0.01%
Covered Lines: 19645
Relevant Lines: 23738

💛 - Coveralls

Copy link
Contributor

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

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

The CI fail is because the vec! macro is now only used in the #[test] module and the unused lint does not look in test. If you change line 21 from #[macro_use] to #[cfg_attr(test, macro_use)] it should fix the issue.

@Kixunil
Copy link
Collaborator

Kixunil commented Aug 23, 2024

Nice!

@jamillambert I think use alloc::vec; in tests module should work too and I'd find it nicer. But it's fine to use attribute too.

@tcharding
Copy link
Member

@jamillambert debugging PRs like a boss, way to go bro, this is super useful.

@ChrisCho-H
Copy link
Contributor Author

Thanks for the reviews! I've added use alloc::vec; in tests module.

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 441aac0 successfully ran local tests

Copy link
Contributor

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

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

ACK 441aac0

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Needs squashing. I have no idea how @apoelstra got to run his per-commit tests without catching this.

@apoelstra
Copy link
Member

Which commit and cargo command fails? I can run cargo test, cargo test --all-features --all-targets, and cargo test --no-default-features --all-targets on each commit. (Using git rebase -x master which isn't quite correct, but that's my habitual "check each commit manually" command.)

My local tests do a more extensive feature matrix (IIRC it tries each individual feature by itself, and also two random subsets of features, where "random" is seeded by the commit ID or something) but it may have missed something.

@Kixunil
Copy link
Collaborator

Kixunil commented Aug 26, 2024

Oh, it's actually warnings, not errors. If you used -D warnings it'd fail but that is maybe too strict for you. If so then I'll ACK this commit even though I would've liked squashing better.

@apoelstra
Copy link
Member

apoelstra commented Aug 26, 2024

If you used -D warnings it'd fail but that is maybe too strict for you.

I've had it on in the past but I've always felt bad nitting new contributors because individual commits don't pass a super strict level of scrutiny, which wouldn't be needed just for bisection to work.. For a similar reason I only run clippy on the final commit of PRs. (Though there I run it with -D warnings, which also picks up compiler warnings.)

It also worses the "sometimes you need to squash unrelated stuff together to make the compiler happy" problem that we sometimes encounter in Rust (and which we did encounter in the final commit of this PR.)

So my ACK stands.

@Kixunil
Copy link
Collaborator

Kixunil commented Aug 26, 2024

Yeah, I thought so. Might be worth to run only some very limited categories of clippy on every commit (basically those that detect a problem with pretty much 100% certainty).

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 441aac0

@apoelstra apoelstra merged commit d7ac6af into rust-bitcoin:master Aug 26, 2024
23 checks passed
@ChrisCho-H ChrisCho-H deleted the feature/optimize-zeroed-vec branch September 20, 2024 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants