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

rust linting: cleanup easy lints - v1 #6303

Closed
wants to merge 15 commits into from

Conversation

jasonish
Copy link
Member

@jasonish jasonish commented Aug 20, 2021

Based on PR: #6299 (ffi lints)

Includes the lint portion of PR #6300 but without rustfmt
changes.

Cleanup all low hanging lints that were mostly fixed automatically
by cargo fix.

Update the clippy suppressions so we are clippy clean by default.
This will make it easier to fix individual lints.

All cases of our transmute can be replaced with more idiomatic
solutions and do no require the power of transmute.

When returning an object to C for life-time management, use
Box::into_raw to convert the boxed object to pointer and use
Box::from_raw to convert back.

For cases where we're just returning a pointer to Rust managed
data, use a cast.
Based on the Rust clippy lint that recommends that any public
function that dereferences a raw pointer, mark all FFI functions
that reference raw pointers with build_slice and cast_pointer
as unsafe.

This commits starts by removing the unsafe wrapper inside
the build_slice and cast_pointer macros then marks all
functions that use these macros as unsafe.

Then fix all not_unsafe_ptr_arg_deref warnings from clippy.

Fixes clippy lint:
https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref
These are needless borrows (references) as the item is already
a reference.
This lint checks for a closure where a function can be directly
supplied.  Runtime performance is unchanged, but this makes
less work for the compiler.
Calling a function in unwrap_or causes that function to always
be called even when not needed. Instead use unwrap_or_else with
a closure which will only be called when needed.
In these simple cases to_string() is recommended and likely
performs better as the formatter is not called.
These add complexity and may not be optimized out by the compiler.
This is a readability fix, as on first look they almost look
like a Rust tuple.
This is code that is not needed and is a bit confusing to see.
When defaulting checked_mul to u64::max, Rust has a method
that does the same thing called saturating_mul.
- ref is discouraged for top level variables
- the other borrow is not required
This is the preffered style and easier to understand the meaning
of the code.
Using `if let` expressions in these cases is better expressed
by the map method, and considered idiomatic Rust for this usage.
Suppress all remaining clippy lints that we trip. This can be
fixed on a per-lint basis.
@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #6303 (138acbc) into master (9d24a53) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6303      +/-   ##
==========================================
+ Coverage   76.94%   76.95%   +0.01%     
==========================================
  Files         611      611              
  Lines      185941   185941              
==========================================
+ Hits       143064   143088      +24     
+ Misses      42877    42853      -24     
Flag Coverage Δ
fuzzcorpus 52.81% <ø> (+0.06%) ⬆️
suricata-verify 51.15% <ø> (-0.01%) ⬇️
unittests 63.12% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

WARNING:

field test baseline %
tlpr1_alerts_cmp

Pipeline 3871

@victorjulien victorjulien mentioned this pull request Aug 23, 2021
@victorjulien
Copy link
Member

Merged in #6304, thanks!

@jasonish jasonish deleted the rust-lint/v1 branch August 23, 2021 18:19
jasonish pushed a commit to jasonish/suricata that referenced this pull request Sep 11, 2023
victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants