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

Use DeviceInt* instead of DeviceUint*. #3291

Merged
merged 1 commit into from Nov 15, 2018

Conversation

Projects
None yet
6 participants
@nical
Collaborator

nical commented Nov 9, 2018

We currently use a mix of both which can be untidy at places and is error prone when doing intermediate computations that may go through negative values.


This change is Reviewable

@nical

This comment has been minimized.

Collaborator

nical commented Nov 9, 2018

Sorry for the antisocial patch. The change is completely mechanical but it ended up touching quite a bit of code and will likely conflict with everything else.

As discussed in the gfx daily, the idea is to use signed integers everywhere instead of a mix of signed and unsigned integers for device pixels. This removes an old TODO in the code that was already complaining about the mess of mixing all kinds of scalar types. This will also help avoid the kind of mistakes that got #3272 backed out (intermediate computations potentially using negative values). For reference, gecko uses signed integers for all of its integer point/rect/size types. Also close to the webrender API the gecko side ended up converting all of the signed integers into unsigned ones (in most places somewhat silently "thanks" to C++) so it adds to my motivation of consistently using i32.

I'm working on the corresponding gecko patch since this affects the API.

@nical nical force-pushed the nical:signed-coords branch 5 times, most recently from 28190f5 to 0d53439 Nov 9, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 9, 2018

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

@kvark

kvark approved these changes Nov 9, 2018

@nical nical force-pushed the nical:signed-coords branch from 42e3afd to b2abb02 Nov 15, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 15, 2018

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

@nical nical force-pushed the nical:signed-coords branch from b2abb02 to 20766c6 Nov 15, 2018

@nical

This comment has been minimized.

Collaborator

nical commented Nov 15, 2018

Green build-only try push: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=210593072&revision=d142cdb56ff05159e42748b10a163204ec141a76

There was a prior push that passed the tests on linux but didn't build everywhere before I did the build fixes: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=210593072&revision=5e79214b6ec27704581198ce0b7d7b2b2feb813e

Unfortunately this PR bitrots faster than I can test it on the CI, but since it is only changing tons of u32s into i32s I'm confident that it's in a good shape now.

@jrmuizel

This comment has been minimized.

Contributor

jrmuizel commented Nov 15, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 15, 2018

📌 Commit 20766c6 has been approved by jrmuizel

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 15, 2018

⌛️ Testing commit 20766c6 with merge 31fd97c...

bors-servo added a commit that referenced this pull request Nov 15, 2018

Auto merge of #3291 - nical:signed-coords, r=jrmuizel
Use DeviceInt* instead of DeviceUint*.

We currently use a mix of both which can be untidy at places and is error prone when doing intermediate computations that may go through negative values.

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

This comment has been minimized.

Contributor

bors-servo commented Nov 15, 2018

💔 Test failed - status-appveyor

Use DeviceInt* instead of DeviceUint*.
We currently use a mix of both which can be untiddy at places and is error prone when doing intermediate computations that may go through negative values.

@jrmuizel jrmuizel force-pushed the nical:signed-coords branch from 95d5663 to 5a384b9 Nov 15, 2018

@jrmuizel

This comment has been minimized.

Contributor

jrmuizel commented Nov 15, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 15, 2018

📌 Commit 5a384b9 has been approved by jrmuizel

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 15, 2018

⌛️ Testing commit 5a384b9 with merge 2b64aec...

bors-servo added a commit that referenced this pull request Nov 15, 2018

Auto merge of #3291 - nical:signed-coords, r=jrmuizel
Use DeviceInt* instead of DeviceUint*.

We currently use a mix of both which can be untidy at places and is error prone when doing intermediate computations that may go through negative values.

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

This comment has been minimized.

Contributor

bors-servo commented Nov 15, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: jrmuizel
Pushing 2b64aec to master...

@bors-servo bors-servo merged commit 5a384b9 into servo:master Nov 15, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details
@staktrace

This comment has been minimized.

Contributor

staktrace commented Nov 16, 2018

@nical can you put the Gecko side patch (properly formatted please) somewhere that I can pull?

@nical

This comment has been minimized.

Collaborator

nical commented Nov 16, 2018

@nical nical deleted the nical:signed-coords branch Nov 16, 2018

@Manishearth Manishearth referenced this pull request Nov 21, 2018

Merged

Update webrender #22237

bors-servo added a commit to servo/servo that referenced this pull request Nov 27, 2018

Auto merge of #22237 - Manishearth:wrup, r=pcwalton
Update webrender

Fixes #22232 , once servo/webrender#3327 lands and I update this PR to include it

All the integer casts are due to servo/webrender#3291

<!-- 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/22237)
<!-- Reviewable:end -->

bors-servo added a commit to servo/servo that referenced this pull request Nov 27, 2018

Auto merge of #22237 - Manishearth:wrup, r=pcwalton
Update webrender

Fixes #22232 , once servo/webrender#3327 lands and I update this PR to include it

All the integer casts are due to servo/webrender#3291

<!-- 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/22237)
<!-- Reviewable:end -->

bors-servo added a commit to servo/servo that referenced this pull request Nov 27, 2018

Auto merge of #22237 - Manishearth:wrup, r=pcwalton
Update webrender

Fixes #22232 , once servo/webrender#3327 lands and I update this PR to include it

All the integer casts are due to servo/webrender#3291

<!-- 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/22237)
<!-- Reviewable:end -->

bors-servo added a commit to servo/servo that referenced this pull request Nov 27, 2018

Auto merge of #22237 - Manishearth:wrup, r=pcwalton
Update webrender

Fixes #22232 , once servo/webrender#3327 lands and I update this PR to include it

All the integer casts are due to servo/webrender#3291

<!-- 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/22237)
<!-- Reviewable:end -->

bors-servo added a commit to servo/servo that referenced this pull request Nov 28, 2018

Auto merge of #22237 - Manishearth:wrup, r=pcwalton
Update webrender

Fixes #22232 , once servo/webrender#3327 lands and I update this PR to include it

All the integer casts are due to servo/webrender#3291

<!-- 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/22237)
<!-- Reviewable:end -->

bors-servo added a commit to servo/servo that referenced this pull request Nov 28, 2018

Auto merge of #22237 - Manishearth:wrup, r=pcwalton
Update webrender

Fixes #22232 , once servo/webrender#3327 lands and I update this PR to include it

All the integer casts are due to servo/webrender#3291

<!-- 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/22237)
<!-- Reviewable:end -->

bors-servo added a commit to servo/servo that referenced this pull request Nov 28, 2018

Auto merge of #22237 - Manishearth:wrup, r=pcwalton
Update webrender

Fixes #22232 , once servo/webrender#3327 lands and I update this PR to include it

All the integer casts are due to servo/webrender#3291

<!-- 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/22237)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment