Closed Bug 1455784 Opened 7 years ago Closed 7 years ago

Use a tagged union to shrink StyleSource

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bholley, Assigned: bholley)

Details

Attachments

(3 files, 1 obsolete file)

right now StyleSource is a three-variant enum, with one of them being "None" to avoid consuming a third word. We could turn this all into a one-word Option<ArcUnion<Rule, Declaration>> with the appropriate machinery. I hacked together a quick implementation of ArcUnion. Haven't finished hooking it up to StyleSource, but looking for quick feedback on the approach and ergonomics.
MozReview-Commit-ID: Jxp2A7cj6CV
Attachment #8969836 - Flags: feedback?(manishearth)
Comment on attachment 8969836 [details] [diff] [review] ArcUnion prototype. v1 Review of attachment 8969836 [details] [diff] [review]: ----------------------------------------------------------------- this looks pretty good. I'd love to eventually publish ServoArc and/or get the Rust stdlib to expose enough so that it can be implemented compatibly with sync::Arc ::: servo/components/servo_arc/lib.rs @@ +976,5 @@ > + /// Similar to deref, but uses the lifetime |a| rather than the lifetime of > + /// self, which is incompatible with the signature of the Deref trait. > + #[inline] > + pub fn get(&self) -> &'a T { > + &*self.0 nit: this can just be self.0, immutable references are Copy
Attachment #8969836 - Flags: feedback?(manishearth) → feedback+
Attached patch Part 1 - ArcUnion. v1 (obsolete) — Splinter Review
MozReview-Commit-ID: Jxp2A7cj6CV
Attachment #8970741 - Flags: review?(manishearth)
MozReview-Commit-ID: AT4sud9goGV
Attachment #8970742 - Flags: review?(manishearth)
MozReview-Commit-ID: Jxp2A7cj6CV
Attachment #8970743 - Flags: review?(manishearth)
Attachment #8970741 - Attachment is obsolete: true
Attachment #8970741 - Flags: review?(manishearth)
Comment on attachment 8970743 [details] [diff] [review] Part 1 - ArcUnion. v2 Review of attachment 8970743 [details] [diff] [review]: ----------------------------------------------------------------- ::: servo/components/servo_arc/lib.rs @@ +1021,5 @@ > + First(ArcBorrow<'a, A>), > + Second(ArcBorrow<'a, B>), > +} > + > +impl<A: 'static, B: 'static> ArcUnion<A, B> { might wish to mark some of these methods `#[inline]`
Attachment #8970743 - Flags: review?(manishearth) → review+
(In reply to Manish Goregaokar [:manishearth] from comment #7) > might wish to mark some of these methods `#[inline]` I thought that was unnecessary given that they're generic?
Oh, that's correct. Sorry.
Attachment #8970742 - Flags: review?(manishearth) → review+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: