rust-lang / backtrace-rs

Backtraces in Rust
https://docs.rs/backtrace
Other
526 stars 245 forks source link

Apply some of the `cargo clippy` suggestions #649

Closed nyurik closed 2 days ago

nyurik commented 1 month ago

As @workingjubilee mentioned, no one is really paying for professional maintenance of backtrace-rs, so might as well try to do some volunteer upkeep. Just to keep things tidy.

Note that the code does not seem to have had cargo fmt run in a while, but I don't want to do it as part of code-changing PR.

The enum backtrace::print::PrintFmt {} seem to be requiring #[non_exhaustive], but that may result in some breaking changes? Not certain how this should be handled, and probably in a separate PR.

nyurik commented 1 month ago

thx for the review, will fix it shortly...

workingjubilee commented 1 month ago

I would not consider changing PrintFmt to use the actual #[non_exhaustive] attribute instead of the "non-exhaustive by convention" to be a breaking change. I would not recommend doing it in this PR however, no.

nyurik commented 1 month ago

Note that asserting stacktrace in the unit test is kinda hacky -- skip_inner_frames is included in non-miri output


# On Linux

   0: skip_inner_frames::backtrace_new_should_start_with_call_site_trace
             at /home/nyurik/dev/rust/backtrace-rs/tests/skip_inner_frames.rs:39:13
   1: skip_inner_frames::backtrace_new_should_start_with_call_site_trace::{{closure}}
             at /home/nyurik/dev/rust/backtrace-rs/tests/skip_inner_frames.rs:35:53
   ...

# MIRI

   0: backtrace_new_should_start_with_call_site_trace
             at tests/skip_inner_frames.rs:39:13
   1: backtrace_new_should_start_with_call_site_trace::{closure#0}
             at tests/skip_inner_frames.rs:35:53
   ...
workingjubilee commented 2 weeks ago

The test being hacky is fine, all tests are allowed to be hacky forever and ever as long as the test is correctly testing at least what it is supposed to test. However something merge conflicted with this, sorry about that.

nyurik commented 2 weeks ago

@workingjubilee fixed