Skip to content

Do not require mutability when operating on Device#343

Merged
mrobinson merged 1 commit into
mainfrom
do-not-require-mutability
Nov 11, 2025
Merged

Do not require mutability when operating on Device#343
mrobinson merged 1 commit into
mainfrom
do-not-require-mutability

Conversation

@mrobinson

Copy link
Copy Markdown
Member

No backend needs this to be mutable, so we can remove the mutability
qualifier from all implementations.

Signed-off-by: Martin Robinson mrobinson@igalia.com

@mrobinson
mrobinson force-pushed the do-not-require-mutability branch from edd9802 to b7044c2 Compare November 11, 2025 10:17
@sagudev

sagudev commented Nov 11, 2025

Copy link
Copy Markdown
Member

It's possible (but I have not checked) that we use &mut not for mutability but for exclusiveness, so that one cannot write the code wrong (otherwise we would need locks internally). So we needs to check if that is the case and if our api is still sound after this change.

@mrobinson

Copy link
Copy Markdown
Member Author

Wouldn't that be possible by simply making sure that Sync isn't implementable?

@sagudev

sagudev commented Nov 11, 2025

Copy link
Copy Markdown
Member

Wouldn't that be possible by simply making sure that Sync isn't implementable?

Not really if it would return types that would refer to lifetime of &mut, but that's not the case here so we only need to check if it's sound from multi threaded perspective. I think most stuff in surfman is not send/sync anyway (because of pointers) so this PR should be OK (I only did a quick glance).

@mrobinson

Copy link
Copy Markdown
Member Author

Wouldn't that be possible by simply making sure that Sync isn't implementable?

Not really if it would return types that would refer to lifetime of &mut, but that's not the case here so we only need to check if it's sound from multi threaded perspective. I think most stuff in surfman is not send/sync anyway (because of pointers) so this PR should be OK (I only did a quick glance).

Yeah, at least on macOS, Device isn't Send, but this may depend on the platform itself.

No backend needs this to be mutable, so we can remove the mutability
qualifier from all implementations.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>
@mrobinson
mrobinson force-pushed the do-not-require-mutability branch from b7044c2 to 1791062 Compare November 11, 2025 11:59
@mrobinson
mrobinson requested a review from sagudev November 11, 2025 12:04

@sagudev sagudev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As far as I see this looks sound. Maybe @jdm should take another look, as I am not familiar with all surfman.

@mrobinson
mrobinson added this pull request to the merge queue Nov 11, 2025
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Nov 11, 2025
@jdm
jdm added this pull request to the merge queue Nov 11, 2025
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 11, 2025
@mrobinson
mrobinson added this pull request to the merge queue Nov 11, 2025
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 11, 2025
@mrobinson

Copy link
Copy Markdown
Member Author

It's hard to see how removing unnecessary mutability qualifiers would cause the tests to start failing. I suspect that CI might be broken, unless I'm missing something here.

@mrobinson
mrobinson added this pull request to the merge queue Nov 11, 2025
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 11, 2025
@sagudev

sagudev commented Nov 11, 2025

Copy link
Copy Markdown
Member

Yes, CI is flaky, but we are currently having trouble with mac13 burn out so: #344

@mrobinson
mrobinson added this pull request to the merge queue Nov 11, 2025
Merged via the queue into main with commit 00e3965 Nov 11, 2025
32 checks passed
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.

3 participants