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

pass lints #35

Closed
wants to merge 1 commit into from
Closed

pass lints #35

wants to merge 1 commit into from

Conversation

placrosse
Copy link

No description provided.

@placrosse
Copy link
Author

Clément, this p.r. simply has changes from running the default rustfmt, as well as implementing the changes required for all clippy lints/warnings to pass. Your test suite appears to pass. I'm not yet 100% confident in the results, however. Your further review appreciated.

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, I accept most of the changes that aren't breakings and that aren't related to the methods of Poly/Database.

@@ -148,7 +148,7 @@ impl PolyDatabase {
/// # Ok(()) }
/// ```
pub fn get<'a, 'txn, T, KC, DC>(
&self,
self,
Copy link
Member

Choose a reason for hiding this comment

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

I believe all those &self into self changes comes from the trivially_copy_pass_by_ref clippy lint?

I understand that, for performance reason, it could be better to passe self by copy rather than by ref, but I kept all those refs because in future versions of heed the Poly/Database types could be much bigger and could contain more things (like locks). Those were my though but I made a mistake in the process: I annotated those two types with Copy, that is a mistake.

I would prefer not to update the Poly/Database methods for the moment, however all other changes look OK.

Copy link
Author

@placrosse placrosse Jan 6, 2020

Choose a reason for hiding this comment

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

I was thinking the same thing. Perhaps some types should not be Copy?

@@ -129,8 +131,9 @@ impl EnvOpenOptions {
/// assert_eq!(ret, Some(5));
/// # Ok(()) }
/// ```
/// # Safety
Copy link
Member

@Kerollmops Kerollmops Jan 5, 2020

Choose a reason for hiding this comment

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

This title as automatically been added by clippy but you forgot to add some content.

It could be great if you tell the user that it is unsafe to use unsafe flags of LMDB (i.e. MdbNoSync, MdbNoMetaSync, MdbNoLock and more).

@Kerollmops Kerollmops added this to the v0.20.0 milestone Jan 11, 2023
@AureliaDolo AureliaDolo mentioned this pull request Jul 5, 2023
3 tasks
@Kerollmops
Copy link
Member

Closing because it has been fixed since. However, thank you fro the help 😊

@Kerollmops Kerollmops closed this Nov 26, 2023
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.

2 participants