Model: Unify PartialEq #311

hunger · 2021-07-10T19:40:41Z

Implement PartialEq for Rc and use that for ModelHandle as well
as Value

Wrap a Rc into a SharedModel and implement PartialEq for that.

Wrap Option into the ModuleHandle as well as Value::Model.

sixtyfps_runtime/corelib/model.rs

tronical · 2021-07-10T20:14:16Z

I think that while this change is source incompatible, it is good - especially the centralization of the ptr_eq.

This sheds a bit of light on the fact that there are quite some ways of constructing a modelhandle :). Is that why you removed the From?

hunger · 2021-07-10T20:49:02Z

@tronical: Yes, this definitely needs some thinking before it can go in.

Is there some documentation on the cpp_cmake parts somewhere? Looks like I broke those.

tronical · 2021-07-10T20:55:11Z

It seems to fail deep in the interpreter api through the cmake build. I’ll try to write something about how that plays together on Monday. Sorry about this can of worms :)

tronical · 2021-07-12T11:32:58Z

I think the heart of the error is this:

Error: api/sixtyfps-cpp/generated_include/sixtyfps_interpreter_internal.h:177:9: error: ‘SharedModel’ does not name a type

which indicates that the new Value variant holding SharedModel is failing because SharedModel is not a type known in C++.

tronical · 2021-07-12T11:39:44Z

I think the heart of the error is this:
Error: api/sixtyfps-cpp/generated_include/sixtyfps_interpreter_internal.h:177:9: error: ‘SharedModel’ does not name a type
which indicates that the new Value variant holding SharedModel is failing because SharedModel is not a type known in C++.

Ah, but Value should not be exposed in the generated headers in the first place.

Make sure to never export the Value enum. It might drag in all sorts of unrelated types (as it happened in #311). We have ValueOpaque for that, so exclude Value from the export config explicitly. However since we expose the Value *pointers* for the struct iterators, at least provide a forward declaration.

tronical · 2021-07-12T12:09:46Z

Yeah, somehow this patch triggered that cbindgen thought that Value is a dependency needed. Commit 2d3ae83 fixes that. If you rebase then the CI should be green. I also started adding some docs to docs/development.md.

hunger · 2021-07-12T17:13:04Z

@tronical: Ok, just did a fresh push, let's see what happens.

sixtyfps_runtime/corelib/model.rs

tronical · 2021-07-12T17:34:08Z

@tronical: Ok, just did a fresh push, let's see what happens.

Yay, it’s green now. Only two things left IMO:

(1) typo in the commit message (Hanlde-> Handle)
(2) either break public API but mention in the commit message why (or edit changelog), or keep it - regarding the From impl

ogoffart

I've tagged the change as breaking_change since this change the public API.

The changes here are overall good. But we haven't decide if the next release of the sixtyfps crate will be 0.1.1 or 0.2.

I guess we should wait a bit to see if we have other reasons to break the source compatibility before merging this.

Also changes to public API deserve a changelog entry.

(It would be nice if there was a CI check that could tell if we are breaking semver or touching the public API)

sixtyfps_runtime/corelib/model.rs

ogoffart · 2021-07-13T07:06:01Z

sixtyfps_runtime/corelib/model.rs

+#[derive(
+    PartialEq, derive_more::Deref, derive_more::DerefMut, derive_more::From, derive_more::Into,
+)]
+pub struct ModelHandle<T>(pub Option<SharedModel<T>>);


i wonder if there is a way to remove the inner pub

Anyway, this is a public API breakage.

True, I added something about this to the CHANGELOG now.

ogoffart · 2021-07-13T07:17:36Z

sixtyfps_runtime/corelib/model.rs

+    // We can get:
+    // * false positives: Two different `Model`s get considered the same, even though they are not.
+    //   For this the `self` pointer need to point to the same place. That can only happen if
+    //   `Self` is `()`, which does not make sense for a `Model`. So we ignore this case.


Self could very well be ().

We could imagine for example a impl Model for () that always returns 0 elements.

or a struct MyStaticModel; impl Model for MyStaticModel { ... }.

But anyway, this cannot happen that two different model are considered the same, because the Rc adds an allocation for the refcounts and always allocates. so the data pointer will always be different anyway.

@ogoffart: You are way deeper in this rabbit hole than me, so did I understand this correctly?

But I think you are right, this lint might actually be wrong: I went ahead and filed rust-lang/rust-clippy#7461 with my current understanding. I am pretty sure I missed something, but maybe some improvement to the explanation of that clippy lint will fall out of this:-)

I updated the text, I hope it is a bit better now.

sixtyfps_runtime/corelib/model.rs

sixtyfps_runtime/interpreter/api.rs

Implement PartialEq for Rc<Model> and use that for ModelHandle as well as Value<Model> Wrap a Rc<Model> into a SharedModel and implement PartialEq for that. Wrap Option<SharedModel> into the ModuleHandle as well as Value::Model.

hunger force-pushed the pr/ptr_eq_cleanup branch from 176b972 to b0ea397 Compare July 10, 2021 19:55

tronical reviewed Jul 10, 2021

View reviewed changes

sixtyfps_runtime/corelib/model.rs Show resolved Hide resolved

hunger force-pushed the pr/ptr_eq_cleanup branch from b0ea397 to 02dcb06 Compare July 10, 2021 20:07

tronical reviewed Jul 10, 2021

View reviewed changes

sixtyfps_runtime/corelib/model.rs Show resolved Hide resolved

hunger force-pushed the pr/ptr_eq_cleanup branch from 46fe958 to c459c58 Compare July 12, 2021 17:12

tronical reviewed Jul 12, 2021

View reviewed changes

sixtyfps_runtime/corelib/model.rs Show resolved Hide resolved

ogoffart added the breaking change Anything that probably require a semver bump label Jul 13, 2021

ogoffart reviewed Jul 13, 2021

View reviewed changes

hunger added 2 commits July 14, 2021 21:25

Model: Unify PartialEq

0921d7a

Implement PartialEq for Rc<Model> and use that for ModelHandle as well as Value<Model> Wrap a Rc<Model> into a SharedModel and implement PartialEq for that. Wrap Option<SharedModel> into the ModuleHandle as well as Value::Model.

ModelHandle: Derive PartialEq

533a21f

hunger force-pushed the pr/ptr_eq_cleanup branch from c459c58 to 533a21f Compare July 14, 2021 19:30

hunger closed this Aug 18, 2021

hunger deleted the pr/ptr_eq_cleanup branch August 18, 2021 17:29

hunger mentioned this pull request Jan 3, 2022

Merge Rc::ptr_eq on traits use into a SharedModel #794

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Model: Unify PartialEq #311

Model: Unify PartialEq #311

hunger commented Jul 10, 2021

tronical commented Jul 10, 2021

hunger commented Jul 10, 2021

tronical commented Jul 10, 2021

tronical commented Jul 12, 2021

tronical commented Jul 12, 2021

tronical commented Jul 12, 2021 •

edited

hunger commented Jul 12, 2021

tronical commented Jul 12, 2021

ogoffart left a comment

ogoffart Jul 13, 2021

hunger Jul 14, 2021

ogoffart Jul 13, 2021

hunger Jul 13, 2021

hunger Jul 14, 2021

Model: Unify PartialEq #311

Model: Unify PartialEq #311

Conversation

hunger commented Jul 10, 2021

tronical commented Jul 10, 2021

hunger commented Jul 10, 2021

tronical commented Jul 10, 2021

tronical commented Jul 12, 2021

tronical commented Jul 12, 2021

tronical commented Jul 12, 2021 • edited

hunger commented Jul 12, 2021

tronical commented Jul 12, 2021

ogoffart left a comment

Choose a reason for hiding this comment

ogoffart Jul 13, 2021

Choose a reason for hiding this comment

hunger Jul 14, 2021

Choose a reason for hiding this comment

ogoffart Jul 13, 2021

Choose a reason for hiding this comment

hunger Jul 13, 2021

Choose a reason for hiding this comment

hunger Jul 14, 2021

Choose a reason for hiding this comment

tronical commented Jul 12, 2021 •

edited