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

project-wide tidying, style updates #432

Merged
merged 21 commits into from
Jun 5, 2024
Merged

project-wide tidying, style updates #432

merged 21 commits into from
Jun 5, 2024

Conversation

cpu
Copy link
Member

@cpu cpu commented May 30, 2024

It might be best to review this commit-by-commit. The total overall changeset should have no functional effect - it's exclusively stylistic changes.

  • Removes explicit typing throughout, whenever the compiler can successfully infer the types on its own
  • Fixes redundant prefixing throughout, whenever an item was already in-scope without the prefix
  • Fixes a couple clippy findings that fell out of ^
  • Applies a project-wide formatting pass, including within the ffi_panic_boundary! macro bodies (using the approach ctz used in cargo fmt inside of ffi_panic_boundary! invocations #383 and that we're using in rustls-openssl-compat).
  • Updates make format and make format-check to enforce rust formatting within the ffi_panic_boundary! macros

Resolves #31

@cpu cpu self-assigned this May 30, 2024
@ctz
Copy link
Member

ctz commented May 31, 2024

Good stuff!

cpu added 21 commits June 4, 2024 14:19
Using:

```
sed -i -e 's/ffi_panic_boundary! {/if true {/g' src/*.rs
cargo fmt
sed -i -e 's/if true {/ffi_panic_boundary! {/g' src/*.rs
```
After removing the type hints, this is now a redundant binding:

```
error: redundant redefinition of a binding `cs`
   --> src/connection.rs:412:17
    |
412 |                 let cs = cs;
    |                 ^^^^^^^^^^^^
    |
help: `cs` is initially defined here
   --> src/connection.rs:408:17
    |
408 |             for cs in ALL_CIPHER_SUITES {
    |                 ^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_locals
    = note: `#[deny(clippy::redundant_locals)]` on by default
```
After refactoring out explicit type hints these are now `useless_vec`
findings of the form:

```
error: useless use of `vec!`
   --> src/client.rs:599:20
    |
599 |         let alpn = vec![h1.into(), h2.into()];
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can use an array directly: `[h1.into(), h2.into()]`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_vec
    = note: `-D clippy::useless-vec` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::useless_vec)]`
```
This papers around a `cargo fmt` limitation that leaves code within the
`ffi_panic_boundary!` macros unformatted.

By also adding this to `format-check`, we'll get CI enforcement to
prevent backsliding.
@cpu
Copy link
Member Author

cpu commented Jun 4, 2024

@jsha Would you like me to hold merging this until you've had a chance to review?

@jsha
Copy link
Collaborator

jsha commented Jun 5, 2024

Nope, go ahead and merge, thanks!

@cpu cpu merged commit 5d445f9 into rustls:main Jun 5, 2024
25 checks passed
@cpu cpu deleted the cpu-tidying branch June 5, 2024 11:22
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.

Consider removing all the type annotations on let bindings
3 participants