Skip to content
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

Stop dereferencing followed by reborrowing in match and if let #1139

Merged
merged 2 commits into from
May 15, 2021

Conversation

MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented May 14, 2021

This is the churn part of #1134, initially intended to remove derive(Copy) from enum Fundamental to fit a String in there. Those were just addressing the compiler errors, but there are a lot - and I mean a lot - more cases where objects are first dereferenced (copied) like match *self {} and then later re-(mutably)-borrowed with ref and ref mut in match arms. This is not necessary anymore (both the * and ref/ref mut are simply removed) and makes the code more pleasant to read.

OTOH doing it this way satisfies https://rust-lang.github.io/rust-clippy/master/#pattern_type_mismatch.... Curious to all your thoughts!

If this is okay I'll go ahead and finish the remainder of the offenders. Finished!

Copy link
Member

@sdroege sdroege left a comment

Choose a reason for hiding this comment

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

I think this change is good, yes. It improves readability by removing some unnecessary noise.

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented May 15, 2021

@sdroege Thanks! I think I covered all instances of unnecessarily dereferencing followed by reborrowing, this is now ready for review.

src/analysis/ffi_type.rs Outdated Show resolved Hide resolved
src/analysis/rust_type.rs Outdated Show resolved Hide resolved
Copy link
Member

@sdroege sdroege 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 to me, big improvement :)

Only please don't put & into patterns but instead use ref / ref mut.

@MarijnS95
Copy link
Contributor Author

Only please don't put & into patterns but instead use ref / ref mut.

& is the inverse of ref/ref mut so that won't work ;)

@sdroege
Copy link
Member

sdroege commented May 15, 2021

Yeah so do it the other way around and match *var in these cases. Having & in patterns is very ugly

@MarijnS95
Copy link
Contributor Author

Yeah so do it the other way around and match *var in these cases. Having & in patterns is very ugly

Those are unfortunately exactly the cases I need to make #1134 work. But then we can add ref in there too and it should work. I don't find it particularly ugly though, but who am I to say :)

@sdroege
Copy link
Member

sdroege commented May 15, 2021

I'm quite sure that even was a clippy lint at some point (or maybe it's disabled by default now). You ideally never want to have & in patterns but only use ref / ref mut and match in the other direction if needed.

@MarijnS95
Copy link
Contributor Author

I'm quite sure that even was a clippy lint at some point (or maybe it's disabled by default now). You ideally never want to have & in patterns but only use ref / ref mut and match in the other direction if needed.

Not that I recall, I only know about https://rust-lang.github.io/rust-clippy/master/#pattern_type_mismatch. Changes have been reverted now :)

@sdroege sdroege merged commit 8baeaa4 into gtk-rs:master May 15, 2021
@MarijnS95 MarijnS95 deleted the ref branch May 15, 2021 12:32
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.

2 participants