Fix clippy lints on Rust 1.72 #1108
Conversation
ids1024
force-pushed
the
clippy
branch
2 times, most recently
from
6699361
to
2b08e4a
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1108 +/- ##
==========================================
- Coverage 24.01% 23.98% -0.03%
==========================================
Files 140 140
Lines 21591 21581 -10
==========================================
- Hits 5184 5176 -8
+ Misses 16407 16405 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Good point, given this is a trait object (and has to be), we also can't implement |
Okay, so the only reason I see why we need the inner buffer (and be able to clone it) is to access the data in the render element. I think we can get rid of that completely by removing the memory buffer render element and (re-)using the texture render element. IIRC we did something similar in the wayland render element. But replacing the |
| @@ -335,6 +335,7 @@ impl Instance { | |||
| info!("Created new instance"); | |||
| info!("Enabled instance extensions: {:?}", inner.enabled_extensions); | |||
|
|
|||
| #[allow(clippy::arc_with_non_send_sync)] | |||
There was a problem hiding this comment.
I think this raises a question, do we want to mark the private inner types as send and Sync or add a bunch of allow attributes for these types of cases?
There was a problem hiding this comment.
I think in this case it would be reasonable to implement Send and Sync for the inner type. For the use in EGLBufferReader::new we can't do that since it's a *mut wl_display that it reference counts. (Though that could possibly be done in a different way.)
There was a problem hiding this comment.
// SAFETY (Host Synchronization): InstanceInner is always stored in an Arc, therefore destruction is
// synchronized (since the inner value of an Arc is always dropped on a single thread).// SAFETY: Destruction is externally synchronized (using an internal Arc).
unsafe impl Send for Instance {}
unsafe impl Sync for Instance {}I think this is safe, though it isn't really related to the fact an Arc is used. Without Arc you'd still only be able to destroy the object once, from one thread.
There was a problem hiding this comment.
Dropping an InstanceInner would be safe since it assumes it owns the underlying ash::Instance. Plus if it's Send and Sync the call to vkDestroyInstance is still externally synchronized (by the borrow checker). And ash makes that function unsafe, so you'd need to know the semantics of InstanceInner to not cause undefined behavior.
I think we can mark InstanceInner as Send + Sync and then remove the manual Send + Sync impls on Instance.
Should fix CI build. Uses `Rc<RefCell<_>>` instead of `Arc<Mutex<_>` in `MemoryRenderBuffer`. `MemoryRenderBufferInner` isn't `Send` because `Box<dyn Any>>` isn't. `Send` can be added there if a `Send` bound is added on `TextureId`. But then `GlesTexture` isn't `Send` so that presumably isn't desired in general.
|
Okay, changed to |
Should fix CI build.
I'm not sure about the use of
ArcinMemoryRenderBuffer. It seemsMemoryRenderBufferInnerisn'tSendbecauseBox<dyn Any>>isn't.Sendcan be added there if aSendbound is added onTextureId. But thenGlesTextureisn'tSendso that presumably isn't desired in general.