-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 by Bors] - Make reflect_partial_eq return more accurate results #5210
Changes from 8 commits
1dcbb94
facf9ed
77ca886
c52483f
7819c8c
383f3cd
93f49a1
fc67513
61333ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -369,6 +369,8 @@ pub fn tuple_apply<T: Tuple>(a: &mut T, b: &dyn Reflect) { | |
/// - `b` is a tuple; | ||
/// - `b` has the same number of elements as `a`; | ||
/// - [`Reflect::reflect_partial_eq`] returns `Some(true)` for pairwise elements of `a` and `b`. | ||
/// | ||
/// Returns [`None`] if the comparison couldn't even be performed. | ||
#[inline] | ||
pub fn tuple_partial_eq<T: Tuple>(a: &T, b: &dyn Reflect) -> Option<bool> { | ||
let b = if let ReflectRef::Tuple(tuple) = b.reflect_ref() { | ||
|
@@ -383,7 +385,7 @@ pub fn tuple_partial_eq<T: Tuple>(a: &T, b: &dyn Reflect) -> Option<bool> { | |
|
||
for (a_field, b_field) in a.iter_fields().zip(b.iter_fields()) { | ||
match a_field.reflect_partial_eq(b_field) { | ||
Some(false) | None => return Some(false), | ||
failed @ (Some(false) | None) => return failed, | ||
Some(true) => {} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: For consistency with the others, maybe this should be an if statement? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had the same thought, but I didn't wanted to touch to make changes as simple as possible. But this doesn't seems to be enough change to have it's own PR, so I can do it without any problems. |
||
} | ||
|
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.
Woah, what's this @ syntax called?
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.
https://doc.rust-lang.org/book/ch18-03-pattern-syntax.html#-bindings
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.
The reference doesn't mention the name. It just specifies it as a subset of identifier patterns. The rust book call them "@ bindings".
In traditional functional languages (Haskell, Ocaml), they usually use the
as
keyword. In rust this wouldn't work since it's the explicit casting operator. Looks like the Ocaml reference calls them "alias patterns", which I think is pretty descriptive https://v2.ocaml.org/manual/patterns.html#sss:pat-aliasThere 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.
That was new to me too. I had to check docs when I saw the
@
(at
) pattern bindings. I'm glad I learn something new and useful.