Skip to content

Fix some clippy warnings #208

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

Merged
merged 1 commit into from Jun 23, 2021
Merged

Fix some clippy warnings #208

merged 1 commit into from Jun 23, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jun 22, 2021

No description provided.

Copy link
Collaborator

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the effort. This looks good, maybe without the mentioned or_else.

I usually don't bother with Clippy, because there are too many false-positives / disagreeable suggestions. I had a quick skim, and there are several more sensible suggestions, some stuff it splits hairs over (I don't get what's wrong with v.len() == 0), and some stuff that's outright wrong:

error: this loop never actually loops
   --> src/widget/view/matrix_view.rs:513:28
    |
513 |               let response = 'outer: loop {
    |  ____________________________^
514 | |                 // We forward events to all children, even if not visible
515 | |                 // (e.g. these may be subscribed to an UpdateHandle).
516 | |                 for child in self.widgets.iter_mut() {
...   |
523 | |                 return Response::Unhandled;
524 | |             };
    | |_____________^

... an ugly pattern, but show me a better one (there's been lots of discussion on this, e.g. here).

@@ -96,7 +96,7 @@ impl<W: Widget, D: Directional> Layout for WithLabel<W, D> {
if !self.rect().contains(coord) {
return None;
}
self.inner.find_id(coord).or(Some(self.id()))
self.inner.find_id(coord).or_else(|| Some(self.id()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this makes sense considering self.id() should in-line to self.core.id. Repeated below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put a clippy allow for that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For v.len() == 0 clippy said it's much faster to check for emptiness with .is_empty.
https://rust-lang.github.io/rust-clippy/master/index.html#len_zero

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some structures can ...

It depends on the data type, e.g. a linked list or map structures may be slow here. Vec is always fast.

@ghost
Copy link
Author

ghost commented Jun 23, 2021

I correct what you want. Don't hesitate if you want to silence others warning or change something else.

Copy link
Collaborator

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks. Shall I merge or are you intending to add to this?

@ghost
Copy link
Author

ghost commented Jun 23, 2021

Yeah, you can merge.

@dhardy
Copy link
Collaborator

dhardy commented Jun 23, 2021

I reported the never_loop issue: rust-lang/rust-clippy#7397

@dhardy dhardy merged commit 23d9f61 into kas-gui:master Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant