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

Bump to Rust 1.84 #2001

Merged
merged 5 commits into from
Jan 28, 2025
Merged

Bump to Rust 1.84 #2001

merged 5 commits into from
Jan 28, 2025

Conversation

coolreader18
Copy link
Collaborator

Description of Changes

This was discussed as something we wanted to do before the 1.0 release.

Expected complexity level and risk

1

cc @clockworklabs/devops

@coolreader18 coolreader18 requested a review from gefjon November 19, 2024 19:26
@coolreader18 coolreader18 force-pushed the noa/bump-rust branch 2 times, most recently from ee088e3 to f2a4169 Compare November 19, 2024 21:23
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

Please don't include unrelated changes in PRs like this. As a reviewer, I don't want to sort through what changed as a result of bumping to the new Rust version versus what drive-by fixes you made. If you feel that the unrelated changes are worthwhile (all of them look good to me), please open a separate PR. I would approve "reformat doc comments to correctly indent multi-line list items" in a heartbeat, for example.

crates/bindings-sys/src/lib.rs Show resolved Hide resolved
crates/bindings-sys/src/lib.rs Show resolved Hide resolved
crates/bindings/src/logger.rs Show resolved Hide resolved
crates/bindings/src/rt.rs Show resolved Hide resolved
crates/client-api-messages/src/name/tests.rs Show resolved Hide resolved
crates/table/src/bflatn_from.rs Show resolved Hide resolved
crates/table/src/eq.rs Show resolved Hide resolved
crates/table/src/eq_to_pv.rs Show resolved Hide resolved
crates/table/src/page.rs Show resolved Hide resolved
crates/table/src/row_hash.rs Show resolved Hide resolved
@coolreader18
Copy link
Collaborator Author

All the changes are fixing warnings that were added sometime in the past 4 versions. That's one of the benefits of keeping up to date with stable rust; less churn in updating wrt rustc/clippy warnings (perhaps we could run CI clippy on latest stable?). The doc-comment formatting changes are all related to a new lint complaining about lists not being indented correctly, and all the cfg-related changes are related to the new check-cfgs lint, that makes sure you don't accidentally typo to e.g. a feature that doesn't exist (as in a couple cases here). And yes, rustc's unused-detection was improved, I think to check for when something declared pub isn't actually publically exposed or used anywhere in the crate. The mem::transmute thing is from a new clippy lint requiring that you specify the T, U type parameters, which I'd agree is definitely best practice - makes it easy to see at a glance what the conversion actually is, instead of having to trace the type inference yourself (or to be using an IDE).

@gefjon
Copy link
Contributor

gefjon commented Nov 22, 2024

That's one of the benefits of keeping up to date with stable rust; less churn in updating wrt rustc/clippy warnings (perhaps we could run CI clippy on latest stable?).

Running Clippy on latest stable seems eminently reasonable. Updating our MSRV every month does not. It is very much the point of a Minimum Supported Rust Version that it should be the lowest possible version.

The doc-comment formatting changes are all related to a new lint complaining about lists not being indented correctly, and all the cfg-related changes are related to the new check-cfgs lint, that makes sure you don't accidentally typo to e.g. a feature that doesn't exist (as in a couple cases here). And yes, rustc's unused-detection was improved, I think to check for when something declared pub isn't actually publically exposed or used anywhere in the crate. The mem::transmute thing is from a new clippy lint requiring that you specify the T, U type parameters, which I'd agree is definitely best practice - makes it easy to see at a glance what the conversion actually is, instead of having to trace the type inference yourself (or to be using an IDE).

In the future, please add this level of commentary to the PR description before requesting review.

@coolreader18
Copy link
Collaborator Author

Fair enough; I think I figured it was self-explanatory but obviously it wasn't.

@coolreader18 coolreader18 changed the title Bump to Rust 1.82 Bump to Rust 1.83 Dec 3, 2024
@coolreader18 coolreader18 force-pushed the noa/bump-rust branch 5 times, most recently from 8ce78b1 to c33ea94 Compare December 9, 2024 17:58
@coolreader18 coolreader18 force-pushed the noa/bump-rust branch 2 times, most recently from 30d9c33 to a2063e8 Compare January 6, 2025 20:11
@coolreader18 coolreader18 requested a review from bfops as a code owner January 6, 2025 20:11
Copy link
Collaborator

@bfops bfops left a comment

Choose a reason for hiding this comment

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

Approving the changes in my code-owned files (crates/cli/src/config.rs and crates/cli/src/errors.rs)

@coolreader18 coolreader18 changed the title Bump to Rust 1.83 Bump to Rust 1.84 Jan 13, 2025
@coolreader18 coolreader18 force-pushed the noa/bump-rust branch 2 times, most recently from e73ffb1 to 7bf0a0a Compare January 13, 2025 18:15
@coolreader18 coolreader18 force-pushed the noa/bump-rust branch 4 times, most recently from aea493e to c2c12cd Compare January 17, 2025 18:19
@coolreader18 coolreader18 force-pushed the noa/bump-rust branch 2 times, most recently from 46d5dd6 to 5c1479b Compare January 21, 2025 18:49
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

I am approving the changes to the files I am an owner of. I echo Phoebe's suggestion about adding commentary in the PR description in the future.

@coolreader18 coolreader18 added this pull request to the merge queue Jan 28, 2025
Merged via the queue into master with commit 293aeba Jan 28, 2025
12 of 13 checks passed
@coolreader18 coolreader18 deleted the noa/bump-rust branch January 29, 2025 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants