Optimize msrv calls (again)#17355
Conversation
rustbot
added
the
S-waiting-on-review
|
rustbot has assigned @samueltardieu. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
I will benchmark and profile this (along with my prior PR) tomorrow. |
| An important consideration is that `Msrv::meets` is relatively expensive to | ||
| call, so you should typically match the MSRV at the end of an if let chain, | ||
| after other short-circuiting checks. |
There was a problem hiding this comment.
This is only partially correct. Msrv::meets only slow if the crate has a clippy::msrv attribute which is becoming less common ever since Cargo.toml got the msrv field. This slowness could even be mostly fixed if it were an unavoidable issue.
The biggest issue with it is that it's just not a good filter. The MSRV being set to whatever the current version is very common and makes this filter out nothing. Even for (actively developed) crates which set an MSRV, it's not usually more than a couple years old. This not only makes most checks not filter anything out, but the total number of ineffective filters grows over time.
| && specializes_tostring(cx, deref_self_ty) | ||
| // Since Rust 1.82, the specialized `ToString` is properly called | ||
| && !msrv.meets(cx, msrvs::SPECIALIZED_TO_STRING_FOR_REFS) |
There was a problem hiding this comment.
I wouldn't touch this one. The msrv check is doing the opposite of what we normally do which undermines a large part of the argument for pushing the check later.
| && let Some(args_snip) = closure.fn_arg_span.and_then(|fn_arg_span| fn_arg_span.get_text(cx)) | ||
| && msrv.meets(cx, msrvs::ITERATOR_TRY_FOLD) | ||
| && !is_from_proc_macro(cx, expr) |
There was a problem hiding this comment.
Don't move this one. Taking a snippet for a suggestions should happen after the everything else.
| && let Some(scrutinee_snip) = scrutinee.span.get_text(cx) | ||
| && msrv.meets(cx, msrvs::MATCHES_MACRO) | ||
| && !is_from_proc_macro(cx, expr) |
There was a problem hiding this comment.
Don't move this one as it's a snippet.
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
4 participants
Address this comment, also taking the opportunity to do some more msrv call optimizations and add it to the documentation.
changelog: none