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

style: Use left-to-right indices in the invalidator. #18884

Merged
merged 1 commit into from Oct 16, 2017

Conversation

emilio
Copy link
Member

@emilio emilio commented Oct 14, 2017

This will make easier to create external invalidations that don't point to a combinator,
and also makes reasoning about the invalidator a bit easier.


This change is Reviewable

@emilio
Copy link
Member Author

emilio commented Oct 14, 2017

r? @heycam

@highfive highfive assigned heycam and nox and unassigned heycam Oct 14, 2017
@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/invalidation/element/collector.rs, components/selectors/matching.rs, components/style/invalidation/element/invalidator.rs, components/selectors/parser.rs, components/style/invalidation/element/invalidation_map.rs
  • @canaltinova: components/style/invalidation/element/collector.rs, components/style/invalidation/element/invalidator.rs, components/style/invalidation/element/invalidation_map.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 14, 2017
@highfive
Copy link

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!

@emilio
Copy link
Member Author

emilio commented Oct 14, 2017

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 9fbf0cb with merge 40445d2...

bors-servo pushed a commit that referenced this pull request Oct 14, 2017
style: Use left-to-right indices in the invalidator.

This will make easier to create external invalidations that don't point to a combinator,
and also makes reasoning about the invalidator a bit easier.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18884)
<!-- Reviewable:end -->
@emilio
Copy link
Member Author

emilio commented Oct 15, 2017

@bors-servo
Copy link
Contributor

@emilio
Copy link
Member Author

emilio commented Oct 15, 2017

Rebased.

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 2819ba5 with merge 8e8fdd8...

bors-servo pushed a commit that referenced this pull request Oct 15, 2017
style: Use left-to-right indices in the invalidator.

This will make easier to create external invalidations that don't point to a combinator,
and also makes reasoning about the invalidator a bit easier.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18884)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

Copy link
Contributor

@heycam heycam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for making selector indexes a bit easier to understand. :-)

@@ -442,10 +442,9 @@ impl<Impl: SelectorImpl> Selector<Impl> {

/// Returns the combinator at index `index` (one-indexed from the right),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this now be "zero-indexed"?

@@ -460,16 +459,24 @@ impl<Impl: SelectorImpl> Selector<Impl> {
self.0.slice.iter()
}

/// Returns the combinator at index `index` (one-indexed from the right),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be "zero-indexed from the left"?

debug_assert!(!dependency.affects_later_siblings());
self.descendant_invalidations.push(Invalidation::new(
dependency.selector.clone(),
dependency.selector_offset,
dependency.selector.len() - dependency.selector_offset + 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment to Invalidation's offset field saying that it's a parse order offset?

This will make easier to create external invalidations, and also makes reasoning
about the invalidator a bit easier.
@emilio
Copy link
Member Author

emilio commented Oct 16, 2017

@bors-servo r=heycam

@bors-servo
Copy link
Contributor

📌 Commit f1cc225 has been approved by heycam

@highfive highfive assigned heycam and unassigned nox Oct 16, 2017
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 16, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit f1cc225 with merge a759ded...

bors-servo pushed a commit that referenced this pull request Oct 16, 2017
style: Use left-to-right indices in the invalidator.

This will make easier to create external invalidations that don't point to a combinator,
and also makes reasoning about the invalidator a bit easier.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18884)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: heycam
Pushing a759ded to master...

@bors-servo bors-servo merged commit f1cc225 into servo:master Oct 16, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants