-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: don't suppress static_mut_refs globally - v2 #12673
Conversation
Other than some debug output on initialization we don't take mutable references to these static mut's. = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html> = note: mutable references to mutable statics are dangerous; it's undefined behavior if any other pointer to the static is used or if any other reference is created for the static while the mutable reference lives Ticket: OISF#7417
It doesn't appear to be needed. The vec being cleared is only set once per run, so never needs to be cleared. Removes one point where we have to supress the static_mut_refs compiler warning. Ticket: OISF#7417
Allows us to get rid of the global supression. Ticket: OISF#7417
As references to static mutables are highly discouraged, remove the global suppressing of the compiler warning. Each use case can be suppressed as needed. Ticket: OISF#7417
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12673 +/- ##
=======================================
Coverage 80.77% 80.77%
=======================================
Files 932 932
Lines 259517 259521 +4
=======================================
+ Hits 209629 209633 +4
Misses 49888 49888
Flags with carried forward coverage won't be shown. Click here to find out more. |
Information: QA ran without warnings. Pipeline 24895 |
// Suppress static_mut_refs for this debug code. Safe | ||
// otherwise. | ||
#[allow(unused_lints, static_mut_refs)] | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to contain this stuff in a helper func that can be inlined?
e.g. something like (pseudocode)
/* SMB_CFG_MAX_READ_SIZE is set at Suricata startup and is static after that... */
#[always_inline]
#[allow(static_mut_refs)]
fn get_max_read_size(...) -> u32 {
SMB_CFG_MAX_READ_SIZE
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that way we can keep the clutter out of the main logic. Esp when addition indents are added things are getting ugly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, and in turn, wrapping the get in a function allows the compiler to reason about it better and you can remove the allow for static_mut_refs.
Replaced by #12680 |
As its use is now a default compiler warning, and eventually will become an
error, remove the global suppression of it. Instead suppress as needed, so its
more intentional.
For reference:
https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html,
Ticket: https://redmine.openinfosecfoundation.org/issues/7417