Can derive Eq #919
Can derive Eq #919
Conversation
|
☔ The latest upstream changes (presumably #920) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Just find out the documentation of Union of what trait can be derived: Looks like this approach doesn't work for Eq on Union. I think it needs a separate analysis. |
src/ir/analysis/has_float.rs
Outdated
| trace!("constrain: {:?}", id); | ||
|
|
||
| if self.has_float.contains(&id) { | ||
| trace!(" already know it do not have array"); |
src/ir/analysis/has_float.rs
Outdated
| let inner_ty = self.ctx.resolve_type(t).canonical_type(self.ctx); | ||
| match *inner_ty.kind() { | ||
| TypeKind::Float(..) | | ||
| TypeKind::Complex(..) => { |
There was a problem hiding this comment.
Instead of directly inspecting the type, shouldn't this look at the incremental results?
if self.has_float.contains(&inner_ty) {
return self.insert(id);
}
ConstrainResult::SameAn array of a struct that contains a float should not derive(Eq), right?
src/ir/context.rs
Outdated
| /// Compute whether the type has float. | ||
| fn compute_has_float(&mut self) { | ||
| assert!(self.has_float.is_none()); | ||
| self.has_float = Some(analyze::<HasFloat>(self)); |
There was a problem hiding this comment.
This should be conditionally computed depending on whichever options enable deriving something that depends on this information. Right now that is only self.options.derive_eq, right?
src/ir/context.rs
Outdated
| /// Look up whether the item with `id` has array or not. | ||
| pub fn lookup_item_id_has_float(&self, id: &ItemId) -> bool { | ||
| assert!(self.in_codegen_phase(), | ||
| "We only compute has array when we enter codegen"); |
src/lib.rs
Outdated
| @@ -731,6 +735,12 @@ impl Builder { | |||
| self | |||
| } | |||
|
|
|||
| /// Set whether `Eq` should be derived by default. | |||
| pub fn derive_eq(mut self, doit: bool) -> Self { | |||
| self.options.derive_eq = doit; | |||
There was a problem hiding this comment.
We can't compute derive(Eq) unless we're also doing derive(PartialEq), so if doit is true, then this method should also set that option to true, and the documentation should describe this behavior.
And inside derive_partial_eq(), if its parameter is false, then it should also set otions.derive_eq to false, and its doc comment should describe that behavior too.
src/options.rs
Outdated
| @@ -79,6 +79,9 @@ where | |||
| Arg::with_name("with-derive-partialeq") | |||
| .long("with-derive-partialeq") | |||
| .help("Derive partialeq on any type."), | |||
| Arg::with_name("with-derive-eq") | |||
| .long("with-derive-eq") | |||
| .help("Derive eq on any type."), | |||
There was a problem hiding this comment.
We should mention here that this also implies --with-derive-partialeq.
|
☔ The latest upstream changes (presumably #924) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@photoszzt what's the status here? We talked a little on IRC and I don't remember the conclusions we came to... is this ready for final review and merging or is there outstanding work to be done? |
|
@fitzgen It's ready for review. The Eq on Union needs manual implementation of PartialEq which doesn't solve by this patch. |
And which presumably requires contextual/domain knowledge about the usage of the union. Ok. |
|
@bors-servo r+ |
|
📌 Commit ff9d000 has been approved by |
|
☀️ Test successful - status-travis |
Fix: #880 r? @fitzgen