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

Remove Root::r() and uses of it #6590

Closed
michaelwu opened this issue Jul 10, 2015 · 5 comments
Closed

Remove Root::r() and uses of it #6590

michaelwu opened this issue Jul 10, 2015 · 5 comments

Comments

@michaelwu
Copy link
Contributor

@michaelwu michaelwu commented Jul 10, 2015

Root implements Deref, so r() isn't necessary.

Deref on Root was implemented in #6150

@fitzgen
Copy link
Member

@fitzgen fitzgen commented Jul 12, 2015

Working on this.

@fitzgen
Copy link
Member

@fitzgen fitzgen commented Jul 12, 2015

Ok, so I made some progress, but quickly got bogged down in the following issue:

We implement .r() not only on Root but also on Option<Root<T>> and Option<Option<Root<T>>> via the helper traits RootedReference<T> and OptionalRootedReference<T>. Those don't have Deref impl'd, and IIUC can't have Deref impl'd because neither Deref nor Option are owned by servo.

Does it make sense to remove .r() usage only from Root and not the (just as common) others? I feel it may be worth preserving the consistency that exists now.

To be honest, most of the changes I've done so far (before I got bogged down in codegen and the above issue) haven't been of the form

foo.r().bar() --> foo.bar()

but of the form

func(foo.r()) --> func(&*foo)

While the former is surely desirable, the latter feels a whole lot more "meh", especially in light of the inconsistency that will arise when dealing with Option<Root<T> and Option<Option<Root<T>> which will still need to use func(foo.r()).

Thoughts?

(cc @jdm who I was pointed towards on irc)

@michaelwu
Copy link
Contributor Author

@michaelwu michaelwu commented Jul 12, 2015

FWIW, I don't really like RootedReference or OptionalRootedReference. Had too many changes in the smup already so I didn't experiment with removing them, but those traits always struck me as a bit odd.

@frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Jul 22, 2015

I don't have any grand thoughts on the questions raised, but if work has already been put into this, it might be worth opening a PR just so it doesn't get bitrotted (as a result this PR would be split up into multiple PRs)

@jdm
Copy link
Member

@jdm jdm commented Jul 28, 2015

Given that this isn't as clear-cut as originally believed, and since I agree with @fitzgen's ambivalence about the foo.r() -> &*foo transformation, I'm inclined to close this.

@jdm jdm closed this Jul 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.