chore: a few small improvements to code quality #9279
Conversation
There was a problem hiding this comment.
Thanks for addressing for going above and beyond with improving the code! The destructuring of arguments is a really nice change.
The symbol comparison changes however are not a code improvement. as_str on symbols is not a no-op, but rather an indexing of a vector requiring to acquire a lock first. See here. Interning an already interned string is pretty much the same, just that it does a HashMap lookup. But then the string comparison is more expensive than the symbol comparison, because the symbol comparison is just an integer comparison. Because those check_* functions are ran on every expr/stmt/item/..., it is in the end cheaper to intern the string once and then do a symbol comparison rather than a string comparison.
(The as_str function additionally goes through a with_session_globals, but I don't know how expensive that is).
TL;DR: Please revert the symbol changes, the rest is great.
clippy_lints/src/map_err_ignore.rs
Outdated
| // only work if the method name is `map_err` and there are only 2 arguments (e.g. x.map_err(|_|[1] | ||
| // Enum::Variant[2])) | ||
| if method.ident.as_str() == "map_err" && args.len() == 2 { | ||
| if method.ident.as_str() == "map_err" { |
There was a problem hiding this comment.
This should then also be changed:
| if method.ident.as_str() == "map_err" { | |
| if method.ident == sym!(map_err) { |
|
@flip1995 Okay. Having looked closer, it's basically |
|
Another non-perf related advantage is, that if a symbol should get pre-inserted by rustc, we have a lint that will tell us to remove the Yeah, your right, it might have equal performance. It probably doesn't make much of a difference. But I'd rather keep the |
Signed-off-by: tabokie <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
|
Perfect, thank you! @bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Some improvements:
is_unit_typeCallorMethodCallwhenever possiblechangelog: none
r? @flip1995