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.

Sign up for GitHub

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

Jump to bottom

fix(clippy): only expose pub functions that need to be public #277

Merged
merged 2 commits into from
Jul 24, 2024
Merged

fix(clippy): only expose pub functions that need to be public #277

merged 2 commits into from
Jul 24, 2024

Conversation

Manuthor
Copy link
Contributor

@Manuthor Manuthor commented Jul 15, 2024 •
edited
Loading

  • do not expose functions as pub if not necessary (lint unreachable-pub)
  • remove useless async (lint unused_async)
  • remove unused self (lint unused_self)
  • fix lint or_fun_call:
    • The function will always be called. This is only bad if it allocates or does some non-trivial amount of work.
  • fix some nursery clippy lints (lint clippy::nursery)
  • fix lint manual_let_else and if_not_else for readability
  • some fixes come from Cargo.toml edition in 2024 (all env. variables functions handling for example)

@Manuthor Manuthor force-pushed the fix_unreachable_pub branch from 25bfc87 to e95d12f Compare July 15, 2024 19:32
@Manuthor Manuthor changed the title fix: only expose pub functions that need to be public fix(clippy): only expose pub functions that need to be public Jul 16, 2024
@Manuthor Manuthor force-pushed the fix_unreachable_pub branch from e95d12f to 57df732 Compare July 16, 2024 05:19
@Manuthor Manuthor requested a review from tbrezot July 16, 2024 05:40
@Manuthor Manuthor force-pushed the fix_unreachable_pub branch from 57df732 to a653a77 Compare July 16, 2024 16:33
tbrezot
tbrezot reviewed Jul 22, 2024
View reviewed changes
crate/cli/src/actions/login.rs Outdated
@@ -114,7 +114,7 @@ pub struct LoginState {

impl LoginState {
#[must_use]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why there is a must-use here since there is not copy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right and furthermore function is never used.

crate/kmip/src/crypto/symmetric/aes_256_gcm.rs
Comment on lines +148 to 149
.unwrap_or_else(|| vec![0_u8; AES_256_GCM_MAC_LENGTH])
.try_into()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks strange to initialize to an empty vec only to convert it right away. Maybe the conversion can come before the unwrapping to allow initializing an empty array directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed. But this initialization happens only if authenticated_encryption_tag is None.

crate/pkcs11/sys/src/lib.rs
@@ -14,6 +14,7 @@

#![allow(non_camel_case_types)]
#![allow(non_snake_case)]
#![allow(clippy::unreadable_literal)]
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it necessary? Maybe add a comment

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

crate/server/src/core/implementation.rs
@@ -86,7 +87,6 @@ impl KMS {
/// - "_kk"
/// - the KMIP cryptographic algorithm in lower case prepended with "_"
pub(crate) fn create_symmetric_key_and_tags(
&self,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a performance improvement but is a rather big change: isn't it better to preserve the symmetry/usual interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Since create_symmetric_key_and_tags is only use once (and locally), I would rather change it now.

@Manuthor Manuthor force-pushed the fix_unreachable_pub branch from 1210b83 to e501984 Compare July 23, 2024 12:05
@Manuthor Manuthor force-pushed the fix_unreachable_pub branch from e501984 to 3011860 Compare July 23, 2024 19:13
@Manuthor Manuthor force-pushed the fix_unreachable_pub branch from 3011860 to d5aff03 Compare July 23, 2024 19:38
@Manuthor Manuthor merged commit 8a7e2ba into develop Jul 24, 2024
35 checks passed
@Manuthor Manuthor deleted the fix_unreachable_pub branch July 24, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbrezot tbrezot tbrezot left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Manuthor @tbrezot