-
Notifications
You must be signed in to change notification settings - Fork 30
Check clippy::large_stack_frames
(blocked on next release)
#241
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
base: main
Are you sure you want to change the base?
Conversation
template/.clippy.toml
Outdated
@@ -1 +1,5 @@ | |||
stack-size-threshold = 1024 | |||
#IF option("ble-trouble") | |||
stack-size-threshold = 2096 |
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's a weird number 😅
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.
yeah - maybe 3072 would be still better than nothing?
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'd be less surprised with 2048, I don't mind the actual value just 2048 + 48 is a weird one
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.
What if we allow
the lint on the main function, instead of guessing a value that will break anyway?
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.
the vanilla freshly generated project fails with "this function may allocate 2056 bytes on the stack"
I was considering to allow it instead for ble-trouble - but even 3096 would still keep people from allocating 74k stack frames 🤔
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.
Sure thing, but memory in main()
isn't that different from static variables, except that the linker doesn't know about it. My thinking is: people can and will happily allocate buffers in main, why not? But if we have a random number in place and their solution is to disable the check globally, then we didn't end up helping. On the other hand, if we allow
main to be whatever it wants to be, the user will be less inclined to just disable the check.
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.
Although I guess this depends on how often people overflow the stack on the main core in fn main
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.
true - especially to avoid static mut
I guess we can allow it with a reason explaining they should still be careful ..... only problem right now: it seems the #[esp_rtos::main]
macro doesn't preserve the attributes 🤔
then we should probably fix that first and let this sit for a while until the next HAL release
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.
Sounds good
template/.clippy.toml
Outdated
@@ -0,0 +1,5 @@ | |||
#IF option("ble-trouble") | |||
stack-size-threshold = 2096 |
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.
not ideal but Trouble needs more
clippy::large_stack_frames
clippy::large_stack_frames
(blocked on next release)
Enables https://rust-lang.github.io/rust-clippy/master/index.html#large_stack_frames with a threshold of 1024
While the lint is not very accurate it might give users an indication and it's easy enough to disable it