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

Update rust-toolchain version #1613

Merged
merged 21 commits into from
Mar 18, 2022

Conversation

remoun
Copy link
Contributor

@remoun remoun commented Mar 12, 2022

Fixes #1408. This includes a newer version of Clippy, so fix those findings, one per commit.

I tried updating to a more recent nightly, but ran into an unknown issue consistently causing Clippy to crash; I filed rust-lang/rust-clippy#8527.

@remoun remoun requested review from jcape, cbeck88 and a team March 12, 2022 04:03
@remoun remoun self-assigned this Mar 12, 2022
Copy link
Contributor

@iamalwaysuncomfortable iamalwaysuncomfortable left a comment

Choose a reason for hiding this comment

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

Looks good, I think most of the #dead code annotations are on variables meant to preserve lifetimes (i.e. the lmdb envs) can be removed with the use of underscore naming.

fog/ingest/enclave/src/lib.rs Outdated Show resolved Hide resolved
fog/ledger/enclave/src/lib.rs Outdated Show resolved Hide resolved
mobilecoind/src/utxo_store.rs Outdated Show resolved Hide resolved
sgx/panic/src/panicking/mod.rs Show resolved Hide resolved
Copy link
Contributor

@iamalwaysuncomfortable iamalwaysuncomfortable left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

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

Generally LGTM, but the couple places in mobilecoind where we are grabbing an LMDB environment handle and hanging on to it but never using it, except in tests can be better represented.

It looks a bit ugly, but IMO the situation is a big ugly ;-)

sgx/panic/src/panicking/mod.rs Show resolved Hide resolved
mobilecoind/src/utxo_store.rs Outdated Show resolved Hide resolved
mobilecoind/src/utxo_store.rs Show resolved Hide resolved
mobilecoind/src/utxo_store.rs Outdated Show resolved Hide resolved
mobilecoind/src/utxo_store.rs Outdated Show resolved Hide resolved
mobilecoind/src/processed_block_store.rs Outdated Show resolved Hide resolved
mobilecoind/src/processed_block_store.rs Outdated Show resolved Hide resolved
mobilecoind/src/processed_block_store.rs Outdated Show resolved Hide resolved
mobilecoind/src/processed_block_store.rs Outdated Show resolved Hide resolved
mobilecoind/src/processed_block_store.rs Outdated Show resolved Hide resolved
jcape
jcape previously approved these changes Mar 17, 2022
Copy link
Contributor

@cbeck88 cbeck88 left a comment

Choose a reason for hiding this comment

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

I'm unsure about removing the Arc<Lmdb:: Environment>. I think that there is some unsoundness in the Lmdb crate and many of the Lmdb::Database objects will crash if all the Environment are collected. I think it might be better to just put _env: ... to silence clippy.

I think the changes in SGX crates look good, we should make some kind of follow up ticket for removing features like panic_unwind, backtraces, since I don't think that stuff is going to happen, and it would let us simplify the code

Generally looks good to me, thank you!

Replace with a shared reference to the Environment returned by the setup helpers.
@remoun remoun requested review from cbeck88 and eranrund March 17, 2022 22:41
@iamalwaysuncomfortable
Copy link
Contributor

Had a nit on the must_use annotations but the context where they are is clear enough. These changed look good to go @garbageslam what say you - this ready to go?

Copy link
Contributor

@iamalwaysuncomfortable iamalwaysuncomfortable left a comment

Choose a reason for hiding this comment

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

Good thoughtful discussion on subtle yet important paradigms in the MC code here, thanks @remoun for bringing these to the forefront and thanks all for the discussion.

Copy link
Contributor

@cbeck88 cbeck88 left a comment

Choose a reason for hiding this comment

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

LGTM

@remoun remoun merged commit 38c55b5 into mobilecoinfoundation:master Mar 18, 2022
@remoun remoun deleted the update-rust-version branch March 18, 2022 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Update rust to more recent nightly
5 participants