Optimize create_mapped_arguments_object and adjust asserts

boa-dev · Sep 21, 2021 · 1813ac7 · 1813ac7
1 parent 40f7a18
commit 1813ac7
Show file tree

Hide file tree

Showing 2 changed files with 16 additions and 22 deletions.
diff --git a/boa/src/builtins/function/arguments.rs b/boa/src/builtins/function/arguments.rs
@@ -139,13 +139,6 @@ impl Arguments {
         obj.borrow_mut().data =
             ObjectData::arguments(Arguments::Mapped(MappedArguments(map.clone())));
 
-        // 12. Let parameterNames be the BoundNames of formals.
-        // 13. Let numberOfParameters be the number of elements in parameterNames.
-        let parameter_names: Vec<_> = formals
-            .iter()
-            .map(|formal| formal.name().to_owned())
-            .collect();
-
         // 14. Let index be 0.
         // 15. Repeat, while index < len,
         for (index, val) in arguments_list.iter().cloned().enumerate() {
@@ -173,14 +166,16 @@ impl Arguments {
         // using a set to optimize `contains`
         let mut mapped_names = FxHashSet::default();
 
+        // 12. Let parameterNames be the BoundNames of formals.
+        // 13. Let numberOfParameters be the number of elements in parameterNames.
         // 18. Set index to numberOfParameters - 1.
         // 19. Repeat, while index ≥ 0,
         // a. Let name be parameterNames[index].
-        for (index, name) in parameter_names.iter().enumerate().rev() {
+        for (index, parameter_name) in formals.iter().map(|fp| fp.name()).enumerate().rev() {
             // b. If name is not an element of mappedNames, then
-            if !mapped_names.contains(name) {
+            if !mapped_names.contains(parameter_name) {
                 // i. Add name as an element of the list mappedNames.
-                mapped_names.insert(name);
+                mapped_names.insert(parameter_name);
                 // ii. If index < len, then
                 if index < len {
                     // 1. Let g be MakeArgGetter(name, env).
@@ -196,7 +191,7 @@ impl Arguments {
                             |_, _, context, captures| {
                                 captures.0.get_binding_value(&captures.1, false, context)
                             },
-                            (env.clone(), name.clone()),
+                            (env.clone(), parameter_name.to_owned()),
                         )
                         .length(0)
                         .name("")
@@ -222,7 +217,7 @@ impl Arguments {
                                     .map(|_| JsValue::Undefined)
                                 // Ok(JsValue::Undefined)
                             },
-                            (env.clone(), name.clone()),
+                            (env.clone(), parameter_name.to_owned()),
                         )
                         .length(1)
                         .name("")

diff --git a/boa/src/object/internal_methods/arguments.rs b/boa/src/object/internal_methods/arguments.rs
@@ -16,7 +16,7 @@ pub(crate) static ARGUMENTS_EXOTIC_INTERNAL_METHODS: InternalObjectMethods =
         ..ORDINARY_INTERNAL_METHODS
     };
 
-/// [[GetOwnProperty]] for arguments exotic objects.
+/// `[[GetOwnProperty]]` for arguments exotic objects.
 ///
 /// More information:
 ///  - [ECMAScript reference][spec]
@@ -64,7 +64,7 @@ pub(crate) fn arguments_exotic_get_own_property(
     ))
 }
 
-/// [[DefineOwnProperty]] for arguments exotic objects.
+/// `[[DefineOwnProperty]]` for arguments exotic objects.
 ///
 /// More information:
 ///  - [ECMAScript reference][spec]
@@ -121,17 +121,16 @@ pub(crate) fn arguments_exotic_define_own_property(
         if desc.is_accessor_descriptor() {
             // i. Call map.[[Delete]](P).
             map.__delete__(&key, context)?;
-
+        }
         // b. Else,
-        } else {
+        else {
             // i. If Desc.[[Value]] is present, then
             if let Some(value) = desc.value() {
                 // 1. Let setStatus be Set(map, P, Desc.[[Value]], false).
-                let set_status = map.set(key.clone(), value, false, context)?;
+                let set_status = map.set(key.clone(), value, false, context);
 
-                // 2. Assert: setStatus is true because formal parameters mapped by argument
-                // objects are always writable.
-                debug_assert!(set_status)
+                // 2. Assert: setStatus is true because formal parameters mapped by argument objects are always writable.
+                assert_eq!(set_status, Ok(true))
             }
 
             // ii. If Desc.[[Writable]] is present and its value is false, then
@@ -213,10 +212,10 @@ pub(crate) fn arguments_exotic_set(
             .expect("HasOwnProperty must not fail per the spec")
         {
             // a. Let setStatus be Set(map, P, V, false).
-            let set_status = map.set(key.clone(), value.clone(), false, context)?;
+            let set_status = map.set(key.clone(), value.clone(), false, context);
 
             // b. Assert: setStatus is true because formal parameters mapped by argument objects are always writable.
-            debug_assert!(set_status);
+            assert_eq!(set_status, Ok(true));
         }
     }