-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
clippy: Fix several warnings in components/script/dom/bindings
#31945
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
Conversation
@@ -521,8 +522,7 @@ impl<T> Clone for Dom<T> { | |||
impl<T> Clone for LayoutDom<'_, T> { | |||
#[inline] | |||
fn clone(&self) -> Self { | |||
assert_in_layout(); | |||
LayoutDom { value: self.value } | |||
*self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change was talked about in #31894 (comment) but it wasn't applied in the end. In that discussion, @mrobinson proposed doing this instead to avoid removing the assert:
*self | |
assert_in_layout(); | |
{ *self } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @eerii, very helpful!
I double checked for this particular warning, it still persisted after proposed change.
Since assert_in_layout() needed to be kept, I decided to allow it.
Considering those changes and formatting, I decided to create a new commit rather than committing your suggestion, if you don't mind :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, sorry for not realizing before! The error still persist because Clone
has the assert but Copy
doesn't, see #31894 (comment). Maybe the Copy
trait should be removed like it says in the comment? Probably a good idea to wait to hear from a maintainer to see what to do, since this is a bit trickier than it seemed.
And no problem at all about the commit!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does sound like the appropriate thing to do here might be to remove the Copy trait implementation...but only if that's possible. Does Servo still compile with that removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrobinson removing the Copy trait implementation (impl<T> Copy for LayoutDom<'_, T> {}
) caused following compile errors:
error[E0204]: the trait `std::marker::Copy` cannot be implemented for this type
--> components/script/layout_dom/document.rs:34:54
|
23 | document: LayoutDom<'dom, Document>,
| ----------------------------------- this field does not implement `std::marker::Copy`
...
34 | impl<'dom, LayoutDataType: LayoutDataTrait> Copy for ServoLayoutDocument<'dom, LayoutDataType> {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error[E0204]: the trait `std::marker::Copy` cannot be implemented for this type
--> components/script/layout_dom/element.rs:68:54
|
55 | element: LayoutDom<'dom, Element>,
| --------------------------------- this field does not implement `std::marker::Copy`
...
68 | impl<'dom, LayoutDataType: LayoutDataTrait> Copy for ServoLayoutElement<'dom, LayoutDataType> {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error[E0204]: the trait `std::marker::Copy` cannot be implemented for this type
--> components/script/layout_dom/node.rs:73:54
|
51 | pub(super) node: LayoutDom<'dom, Node>,
| -------------------------------------- this field does not implement `std::marker::Copy`
...
73 | impl<'dom, LayoutDataType: LayoutDataTrait> Copy for ServoLayoutNode<'dom, LayoutDataType> {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error[E0204]: the trait `std::marker::Copy` cannot be implemented for this type
--> components/script/layout_dom/shadow_root.rs:30:54
|
19 | shadow_root: LayoutDom<'dom, ShadowRoot>,
| ---------------------------------------- this field does not implement `std::marker::Copy`
...
30 | impl<'dom, LayoutDataType: LayoutDataTrait> Copy for ServoShadowRoot<'dom, LayoutDataType> {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
For more information about this error, try `rustc --explain E0204`.
error: could not compile `script` (lib) due to 4 previous errors
a10dcda
to
5668655
Compare
@@ -418,7 +418,7 @@ where | |||
/// without causing conflicts , unexpected behavior. | |||
/// <https://github.com/servo/mozjs/blob/main/mozjs-sys/mozjs/js/public/ArrayBuffer.h#L89> | |||
unsafe extern "C" fn free_func(_contents: *mut c_void, free_user_data: *mut c_void) { | |||
let _ = Arc::from_raw(free_user_data as _); | |||
let _ = Arc::from_raw(free_user_data as *const _); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the warning, why is *const _
better than _
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Loirooriol, I used *const
, because from_raw
function receives *const T
type as an argument
pub unsafe fn from_raw(ptr: *const T) -> Self {
unsafe { Arc::from_raw_in(ptr, Global) }
}
the warning was:
warning: creating a `Arc` from a void raw pointer
--> components/script/dom/bindings/buffer_source.rs:421:17
|
421 | let _ = Arc::from_raw(free_user_data as _);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: cast this to a pointer of the appropriate type
--> components/script/dom/bindings/buffer_source.rs:421:31
|
421 | let _ = Arc::from_raw(free_user_data as _);
| ^^^^^^^^^^^^^^^^^^^
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#from_raw_with_void_ptr
= note: `#[warn(clippy::from_raw_with_void_ptr)]` on by default
b603bbd
to
9acef18
Compare
Fixed warnings triggered by following checks:
./mach build -d
does not report any errors./mach test-tidy
does not report any errors