-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Remove unnecessary fixme on new thread stack size #122088
Conversation
As the FIXME itself notes, there's nothing to fix here.
Also important is that a 0, which would be rounded up to the nearest page size of 0, actually is treated as the "default thread stack size". Please fix this to the more minimal comment that mentions both of these things, i.e. "this is either at least Does starting a thread using a 4KB stack actually succeed? Do we have a test for this? I would like to see a test if we do not. Obviously, for such a test, the thread doesn't have to do anything meaningful: it's fine if it basically runs |
Sure, I can replace it with a useful comment and add a test. |
This comment has been minimized.
This comment has been minimized.
Thanks! That's fucking wild that that works. @bors r+ rollup |
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#122015 (Add better explanation for `rustc_index::IndexVec`) - rust-lang#122061 (Clarify FatalErrorHandler) - rust-lang#122062 (Explicitly assign constructed C++ classes) - rust-lang#122072 (Refer to "slice" instead of "vector" in Ord and PartialOrd trait impl of slices) - rust-lang#122088 (Remove unnecessary fixme on new thread stack size) - rust-lang#122094 (Remove outdated footnote "missing-stack-probe" in platform-support) - rust-lang#122107 (Temporarily make allow-by-default the `non_local_definitions` lint) - rust-lang#122109 (compiletest: Add a `//@ needs-threads` directive) Failed merges: - rust-lang#122104 (Rust is a proper name: rust → Rust) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#122088 - ChrisDenton:fixme, r=workingjubilee Remove unnecessary fixme on new thread stack size As the FIXME itself notes, there's nothing to fix here. And as the documentation for [`CreateThread`] says of `dwStackSize`, the value is rounded up to the nearest page. A 4kb stack is very small but perfectly usable if you're careful. Of course it will be very limited but there's no reason to add artificial limits. We don't know what the user is doing. [`CreateThread`]: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createthread
As the FIXME itself notes, there's nothing to fix here.
And as the documentation for
CreateThread
says ofdwStackSize
, the value is rounded up to the nearest page. A 4kb stack is very small but perfectly usable if you're careful. Of course it will be very limited but there's no reason to add artificial limits. We don't know what the user is doing.