-
Notifications
You must be signed in to change notification settings - Fork 13
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
Apply clippy changes #36
Conversation
a0b5a75
to
34d44db
Compare
hifive1/src/stdout.rs
Outdated
|
||
struct SerialWrapper(Tx<Uart0>); | ||
|
||
static STDOUT: Mutex<RefCell<Option<SerialWrapper>>> = Mutex::new(RefCell::new(None)); |
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.
Is this really necessary? We only use one hart I don't think we need to Mutex and Refcell if we can just guard against the interrupts.
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.
To be honest, I'm not sure which is the best way to remove static mut
variables for this target now that Clippy will complain about them. I asked in Matrix and got the following suggestions:
- Keep using
static mut
by doing&mut *addr_of_mut!(MY_STATIC_MUT)
instead of&mut MY_STATIC_MUT
. - Adding a
Mutex<RefCell>
.
I guess that the second one is more sound, but given that we only have one HART, a critical section should be enough and we could go for the first option, which will be slightly lighter.
Let me know what you prefer, and I will adapt it :)
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.
I'm a bit too short on time but I'd love to see if the added Mutex check has any visible slowdown when using STDOUT. If not, this becomes moot. My main worry here is that output logging would be slowed down too much, but I suspect it'd have to be some seriously heavy logging for this to be noticable.
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.
I pushed a new version that avoids using mutexes. Let me know what you think.
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.
Great improvements and I won't block this but left one question open.
LGTM! Thanks for taking this on |
No description provided.