Id: don't panic on write error#10769
Conversation
merge main
…tils into id-panic-on-write
|
GNU testsuite comparison: |
| #[allow(clippy::cognitive_complexity)] | ||
| pub fn uumain(args: impl uucore::Args) -> UResult<()> { | ||
| let matches = uucore::clap_localization::handle_clap_result(uu_app(), args)?; | ||
| let mut lock = io::stdout().lock(); |
There was a problem hiding this comment.
I'm taking a first read through and everything seems right, I need to do some more local testing because I am realizing I don't on a deep level understand what would be better, locking it at the beginning of locking it for each write
There was a problem hiding this comment.
So negligible measured perf differences, in theory on write heavy utilities this could have performance implications so this would be the right approach. We just have to be careful on multi-threaded utilities
There was a problem hiding this comment.
As I understand from this, "The print! macro will lock the standard output on each call. If you call print! within a hot loop, this behavior may be the bottleneck of the loop. To avoid this, lock stdout with io::stdout().lock()", locking and unlocking seems to incur some overhead, that's why I opted to lock once.
There was a problem hiding this comment.
Or running many utils at same terminal?
|
GNU testsuite comparison: |
|
Just waiting for the build to finish, the other CI failures are unrelated |
|
Thanks! |
3 participants
fixes #10561
As many functions now return Result<()> instead of (), also got rid of some unwrap()s where possible.