-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: map_entry
FP on struct member
#14151
Conversation
r? clippy |
clippy_lints/src/entry.rs
Outdated
fn is_map_or_parent_used(cx: &LateContext<'_>, map: &Expr<'_>, expr: &Expr<'_>) -> bool { | ||
SpanlessEq::new(cx).eq_expr(map, expr) | ||
|| matches!(map.kind, ExprKind::Field(inner, _) | ExprKind::Index(inner, _, _) if is_map_or_parent_used(cx, inner, expr)) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe for_each_expr
would be nicer.
Note this usage of ||
is clever but hard to read at first.
Also, can you try to document this function a bit? My initial 2min look leaves me a bit confused on mostly the name. I'm not sure if it's misleading or I'm missing context not present in the diff.
I can reroll or look closer later but I believe just the refactoring I've suggested is what this needs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
8d082dd
to
1d3d43c
Compare
clippy_lints/src/entry.rs
Outdated
/// Check if the given expression is used for each sub-expression in the given map. | ||
/// For example, in map `a.b.c.my_map`, The expression `a.b.c.my_map`, `a.b.c`, `a.b`, and `a` are | ||
/// all checked. | ||
fn is_each_expr_in_map_used<'tcx>(cx: &LateContext<'tcx>, map: &'tcx Expr<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_any_expr_in_map_used
is closer to what it does, yes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair
fixes #13934
I modified the part for checking if the map is used so that it can check field and index exprs.
changelog: [
map_entry
]: fix FP on struct member