clippy: Fix various clippy problems in components/scripts/dom
#31910
Conversation
components/script/dom/globalscope.rs
Outdated
| @@ -294,15 +294,15 @@ pub struct GlobalScope { | |||
| /// | |||
| /// <https://html.spec.whatwg.org/multipage/#about-to-be-notified-rejected-promises-list> | |||
| #[ignore_malloc_size_of = "mozjs"] | |||
| uncaught_rejections: DomRefCell<Vec<Box<Heap<*mut JSObject>>>>, | |||
There was a problem hiding this comment.
What was the error that led you to removing the Box here?
There was a problem hiding this comment.
It was saying box is unnecessary
https://rust-lang.github.io/rust-clippy/master/index.html#/vec_box
There was a problem hiding this comment.
I think for now you should just allow the warning for uncaught_rejections and consumed_rejections below. It seems that Heap always comes boxed and it's tricky to undo that. Instead put a comment like:
/// TODO: `Vec` already stores members in a `Box` so this extra box is an unnecessary layer of
/// indirection. Can it be removed?cc @sagudev
There was a problem hiding this comment.
Objects using heap need to be on the heap and they need to be immovable. While vector content is on heap, immovability can only be assured with box. It is also planed to wrap this in pinned box: servo/mozjs#376 and then we could also pack it all in more high level wrapper that would hide all type nesting.
TL;DR: we should definitely keep the box here.
There was a problem hiding this comment.
Yeahhhh i get it now
components/script/dom/globalscope.rs
Outdated
| } | ||
|
|
||
| pub fn remove_uncaught_rejection(&self, rejection: HandleObject) { | ||
| let mut uncaught_rejections = self.uncaught_rejections.borrow_mut(); | ||
|
|
||
| if let Some(index) = uncaught_rejections | ||
| .iter() | ||
| .position(|promise| *promise == Heap::boxed(rejection.get())) | ||
| .position(|promise| *promise == *Heap::boxed(rejection.get())) |
There was a problem hiding this comment.
I'm not sure about this comparison now. Heap::boxed is making a Box<> and then you immediately dereference it. We are boxing the heap unnecessarily now.
components/script/dom/globalscope.rs
Outdated
| @@ -2179,42 +2179,42 @@ impl GlobalScope { | |||
| pub fn add_uncaught_rejection(&self, rejection: HandleObject) { | |||
| self.uncaught_rejections | |||
| .borrow_mut() | |||
| .push(Heap::boxed(rejection.get())); | |||
| .push(*Heap::boxed(rejection.get())); | |||
There was a problem hiding this comment.
This is creating a Box<Heap<>> and then immediately pulling it apart. I would either keep using Box<...> in the vector or figure out a way to create this Heap without putting it in a Box<...>.
components/script/dom/globalscope.rs
Outdated
| @@ -294,15 +294,15 @@ pub struct GlobalScope { | |||
| /// | |||
| /// <https://html.spec.whatwg.org/multipage/#about-to-be-notified-rejected-promises-list> | |||
| #[ignore_malloc_size_of = "mozjs"] | |||
| uncaught_rejections: DomRefCell<Vec<Box<Heap<*mut JSObject>>>>, | |||
There was a problem hiding this comment.
I think for now you should just allow the warning for uncaught_rejections and consumed_rejections below. It seems that Heap always comes boxed and it's tricky to undo that. Instead put a comment like:
/// TODO: `Vec` already stores members in a `Box` so this extra box is an unnecessary layer of
/// indirection. Can it be removed?cc @sagudev
mrobinson
changed the title
components/scripts/dom
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors