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

Move EventTargetTypeId/NodeTypeId to DOMClass #6615

Closed
wants to merge 1 commit into from

Conversation

@michaelwu
Copy link
Contributor

michaelwu commented Jul 13, 2015

Saves 16 bytes in Node, and 8 bytes in EventTarget objects that aren't nodes.

A number of *TypeId enums were adjusted to allow CodegenRust to automatically generate enums.

Review on Reviewable

@highfive
Copy link

highfive commented Jul 13, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Jul 13, 2015

Critic review: https://critic.hoppipolla.co.uk/r/5541

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@michaelwu michaelwu force-pushed the michaelwu:move-typeid branch from 6ff2042 to cea73f2 Jul 13, 2015
@jdm
Copy link
Member

jdm commented Jul 13, 2015

I presume this stored in the NodeInfo struct in Gecko. I'll need to think about this change.

@michaelwu
Copy link
Contributor Author

michaelwu commented Jul 13, 2015

I don't think there's a direct equivalent in Gecko since gecko has vtables to store info about the class hierarchy. I do plan to roll other node fields into static data structures, similar to NodeInfo in gecko. Our equivalent might be fully statically allocated though.

@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 13, 2015

Not a big fan on the face of it...

@bors-servo
Copy link
Contributor

bors-servo commented Jul 16, 2015

The latest upstream changes (presumably #6572) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented Jul 27, 2015

I'm really ambivalent about this change. On one hand, it reduces the size of common objects and automates one more thing, but on the other hand it adds a lot of unreachable!() calls and extra enum variants, as well as extra unsafe blocks. It doesn't feel like a great gain to me.

@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 28, 2015

Ditto.

@metajack
Copy link
Contributor

metajack commented Jul 29, 2015

@michaelwu ping

@michaelwu michaelwu force-pushed the michaelwu:move-typeid branch from cea73f2 to 28ee908 Aug 5, 2015
@jdm
Copy link
Member

jdm commented Aug 5, 2015

I think the ping was more about addressing our concerns rather than rebasing.

@bors-servo
Copy link
Contributor

bors-servo commented Aug 5, 2015

The latest upstream changes (presumably #6416) made this pull request unmergeable. Please resolve the merge conflicts.

@michaelwu
Copy link
Contributor Author

michaelwu commented Aug 5, 2015

No I was just rebasing because I had to. This is necessary for other patches I'm working on and makes more sense in context.

@nox
Copy link
Member

nox commented Sep 11, 2015

Couldn't we introduce a new Servo-specific WebIDL extended attribute to mark interfaces that shouldn't appear in the TypeId hierarchy, to avoid NodeTypeId::Node and similar values? @michaelwu Do you want me to try to do such a thing and otherwise rebase your branch, so you can focus on your magical DOM thing?

@michaelwu
Copy link
Contributor Author

michaelwu commented Sep 11, 2015

@nox I'd be happy with a solution like that.

@nox
Copy link
Member

nox commented Sep 11, 2015

Superseded by #7606.

@nox nox closed this Sep 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.