Implemented Array.prototype.pop().

[dbatyai]

JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai [email protected]

[ruben-ayrapetyan]

Hi Dániel.

index_str used in the line, was freed with ecma_deref_ecma_string.

The ECMA_TRY_CATCH would overwrite ret_value upon exception.
Generally, ret_value should be written only once - with value that would be actually returned from a procedure.

Could you, please, add "_p" suffix to index_str like for other pointer variables?

[dbatyai]

Hi Ruben!

I fixed the issues you mentioned and updated the commit.

[ruben-ayrapetyan]

Hi Dániel.

The ECMA_TRY_CATCH, upon exception, would overwrite ret_value without freeing it, and, so, causing memory leak.

We've recently introduced assertion check for similar cases (May 8, b05d239).
So, in future, precommit testing would check for the issues.

We could move code, writing length property, to a separate function, returning ecma_completion_value_t
('normal undefined' if no exception was thrown, and 'throw completion' - otherwise).
So, we could call it with ECMA_TRY_CATCH in if and else blocks.

  if (len > 0)
+  {
+    len--;
+    /* 5.a */
+    ecma_string_t *index_str_p = ecma_new_ecma_string_from_uint32 (len);
+
+    /* 5.b */
+    ECMA_TRY_CATCH (get_value, ecma_op_object_get (obj_p, index_str_p), ret_value);
+
+    /* 5.c */
+    ECMA_TRY_CATCH (del_value, ecma_op_object_delete (obj_p, index_str_p, true), ret_value);
+    ECMA_TRY_CATCH (set_length_value,
+                    ecma_builtin_array_prototype_helper_set_length (obj_p, len),
+                    ret_value);
+    ret_value = ecma_make_normal_completion_value (ecma_copy_value (get_value, true));
+    ECMA_FINALIZE (set_length_value);
+    ECMA_FINALIZE (del_value);
+
+    ECMA_FINALIZE (get_value)
+
+    ecma_deref_ecma_string (index_str_p);
+  }

[dbatyai]

Hi Ruben!

I've updated the commit as you suggested. Please take a look.

[dbatyai]

I've made a slight change, I believe it's better this way.

[galpeter]

If the .pop does not throw the error, then there will be no assert check and we'll assume that everything is ok. We should add an assert (false); after the .pop call to detect that there should have been an exception.

[dbatyai]

I've updated the test to make sure an exception was thrown by pop, when that's the expected behavior.

[ruben-ayrapetyan]

Hello, Dániel.

It's great, currently the changes seem to be semantically correct.

Now, we can proceed to style questions:

• ECMA pseudocode
  
  It is better to follow ECMA pseudocode as much as possible without considerably impairing quality / readability of code. If code in ECMA description is duplicated in several execution paths, maybe it is worth to put duplicated part to a separate helper function (as you did for setting 'length' property), and not to merge execution paths.

• ECMA_TRY_CATCH style
  
  Maybe it seems more efficient to just perform

  if (!ecma_is_completion_value_normal (completion))
  {
    return completion;
  }
  

  but this makes code less readable and less similar to how other parts of engine are implemented. In future, this would make global changes during engine development more difficult,
  because all special, dissimilar code parts would need to be considered separately.

  So, its better to look on how necessary logic is implemented in nearby engine's area, and consider following the usual style.

  ECMA_TRY_CATCH (val, operation (...), ret_value);
  /* Use val, perform other try-catched operations, etc.
     Set ret_value in the innermost block */
  ECMA_FINALIZE (val);
  
  return ret_value;
  

  It may well be true, that the exception-handling style is not the most readable, efficient, etc.
  Anyway, changes of the style should be performed globally in all engine (may be step by step,
  but purposefully), premilinary discussed, comprehensively analyzed considering necessity, advantages, and timeliness.

• Comments: to any function, any function's arguments; tail comment after any function.

  The purpose of comments is improvement of:

  • code readability, by explaining it, guiding reader;
  • code quality, by describing interfaces (actually, establishing requirement for the interface), making possible to double check correctness of call site and logic of the interface's implementation.
    
    

  Current usual coding style is the following:

  /**
  * /Description of the function/
  * 
  * /If ECMA-defined, with reference to ECMA specification paragraph ('See also:' block)./
  *
  * Note:
  *      /Note on unusual behaviour, arguments meaning, etc., if any.
  *       There can be more than one 'Note:' block./
  *
  * @return /description of return value, if it is not void.
  *
  *          For ECMA-defined operations there can be just
  *          'completion value
  *          Returned value must be freed with ecma_free_completion_value.'.
  *
  *          For other routines there should be brief description on what
  *          the returned value is and what are rules on operating with the value/
  */
  /function's return type/
  /function's name, beginning with module prefix/
  {
    / ... /
  } /* /function's name again/ */
  

Following the list above, I updated your patch.

Please, compare the two versions, considering updated version as supplement to the guideline, and try to follow the style for this and subsequent patches.

diff --git a/jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.cpp b/jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.cpp
index 0d2b436..d0ab6ef 100644
--- a/jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.cpp
+++ b/jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.cpp
@@ -45,6 +45,34 @@
  */

 /**
+ * Helper function to set an object's 'length' property
+ *
+ * @return completion value (the same as returned by the object's [[Put]] method)
+ *         Returned value must be freed with ecma_free_completion_value.
+ */
+static ecma_completion_value_t
+ecma_builtin_array_prototype_helper_set_length (ecma_object_t *object_p, /**< object */
+                                                uint32_t length) /**< value to set (converted to ecma_number_t) */
+{
+  ecma_completion_value_t ret_value = ecma_make_empty_completion_value ();
+
+  ecma_string_t *magic_string_length_p = ecma_get_magic_string (ECMA_MAGIC_STRING_LENGTH);
+
+  ecma_number_t *len_p = ecma_alloc_number ();
+  *len_p = ecma_uint32_to_number (length);
+
+  ret_value = ecma_op_object_put (object_p,
+                                  magic_string_length_p,
+                                  ecma_make_number_value (len_p),
+                                  true);
+
+  ecma_dealloc_number (len_p);
+  ecma_deref_ecma_string (magic_string_length_p);
+
+  return ret_value;
+} /* ecma_builtin_array_prototype_helper_set_length */
+
+/**
  * The Array.prototype object's 'toString' routine
  *
  * See also:
@@ -60,6 +88,87 @@ ecma_builtin_array_prototype_object_to_string (ecma_value_t this_arg) /**< this
 } /* ecma_builtin_array_prototype_object_to_string */

 /**
+ * The Array.prototype object's 'pop' routine
+ *
+ * See also:
+ *          ECMA-262 v5, 15.4.4.6
+ *
+ * @return completion value
+ *         Returned value must be freed with ecma_free_completion_value.
+ */
+static ecma_completion_value_t
+ecma_builtin_array_prototype_object_pop (ecma_value_t this_arg) /**< this argument */
+{
+  ecma_completion_value_t ret_value = ecma_make_empty_completion_value ();
+
+  /* 1. */
+  ECMA_TRY_CATCH (obj_this,
+                  ecma_op_to_object (this_arg),
+                  ret_value);
+
+  ecma_object_t *obj_p = ecma_get_object_from_value (obj_this);
+
+  ecma_string_t *magic_string_length_p = ecma_get_magic_string (ECMA_MAGIC_STRING_LENGTH);
+
+  /* 2. */
+  ECMA_TRY_CATCH (len_value,
+                  ecma_op_object_get (obj_p, magic_string_length_p),
+                  ret_value);
+
+  ECMA_OP_TO_NUMBER_TRY_CATCH (len_number, len_value, ret_value);
+
+  /* 3. */
+  uint32_t len = ecma_number_to_uint32 (len_number);
+
+  /* 4. */
+  if (len == 0)
+  {
+    ECMA_TRY_CATCH (set_length_val,
+                    ecma_builtin_array_prototype_helper_set_length (obj_p, 0),
+                    ret_value);
+
+    ret_value = ecma_make_simple_completion_value (ECMA_SIMPLE_VALUE_UNDEFINED);
+
+    ECMA_FINALIZE (set_length_val);
+  }
+  else
+  {
+    /* 5. */
+    JERRY_ASSERT (len > 0);
+
+    /* 5.a */
+    uint32_t indx = len - 1u;
+    ecma_string_t *index_str_p = ecma_new_ecma_string_from_uint32 (indx);
+
+    /* 5.b */
+    ECMA_TRY_CATCH (get_value, ecma_op_object_get (obj_p, index_str_p), ret_value);
+
+    /* 5.c */
+    ECMA_TRY_CATCH (del_value, ecma_op_object_delete (obj_p, index_str_p, true), ret_value);
+
+    /* 5.d */
+    ECMA_TRY_CATCH (set_length_val,
+                    ecma_builtin_array_prototype_helper_set_length (obj_p, indx),
+                    ret_value);
+
+    ret_value = ecma_copy_completion_value (get_value);
+
+    ECMA_FINALIZE (set_length_val);
+    ECMA_FINALIZE (del_value);
+    ECMA_FINALIZE (get_value);
+
+    ecma_deref_ecma_string (index_str_p);
+  }
+
+  ECMA_OP_TO_NUMBER_FINALIZE (len_number);
+  ECMA_FINALIZE (len_value);
+  ecma_deref_ecma_string (magic_string_length_p);
+  ECMA_FINALIZE (obj_this);
+
+  return ret_value;
+} /* ecma_builtin_array_prototype_object_pop */
+
+/**
  * The Array.prototype object's 'push' routine
  *
  * See also:
diff --git a/jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.inc.h b/jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.inc.h
index 4e09fd7..dd7609f 100644
--- a/jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.inc.h
+++ b/jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.inc.h
@@ -59,6 +59,7 @@ NUMBER_VALUE (ECMA_MAGIC_STRING_LENGTH,
 /* Routine properties:
  *  (property name, C routine name, arguments number or NON_FIXED, value of the routine's length property) */
 ROUTINE (ECMA_MAGIC_STRING_TO_STRING_UL, ecma_builtin_array_prototype_object_to_string, 0, 0)
+ROUTINE (ECMA_MAGIC_STRING_POP, ecma_builtin_array_prototype_object_pop, 0, 0)
 ROUTINE (ECMA_MAGIC_STRING_PUSH, ecma_builtin_array_prototype_object_push, NON_FIXED, 1)

 #undef OBJECT_ID
diff --git a/tests/jerry/array_prototype_pop.js b/tests/jerry/array_prototype_pop.js
new file mode 100644
index 0000000..a741e01
--- /dev/null
+++ b/tests/jerry/array_prototype_pop.js
@@ -0,0 +1,80 @@
+// Copyright 2015 Samsung Electronics Co., Ltd.
+// Copyright 2015 University of Szeged.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+var array = [ "foo", [], Infinity, 4 ];
+assert (array.length === 4);
+
+assert (array.pop() === 4)
+assert (array.length === 3);
+
+assert (array.pop() === Infinity);
+assert (array.length === 2);
+
+var a = array.pop()
+assert (a instanceof Array);
+assert (array.length === 1);
+
+assert (array.pop() === "foo");
+assert (array.length === 0);
+
+assert (array.pop() === undefined);
+assert (array.length === 0);
+
+// Function that throws ReferenceError
+var referenceErrorThrower = function () {
+  throw new ReferenceError ("foo");
+};
+
+// Checking behavior when unable to get length
+var obj = { pop : Array.prototype.pop };
+Object.defineProperty(obj, 'length', { 'get' : referenceErrorThrower });
+
+try {
+  obj.pop();
+  assert(false);
+} catch (e) {
+  assert(e.message === "foo");
+  assert(e instanceof ReferenceError);
+}
+
+// Checking behavior when unable to set length
+var obj = { pop : Array.prototype.pop };
+Object.defineProperty(obj, 'length', { 'set' : referenceErrorThrower });
+
+try {
+  obj.pop();
+  assert(false);
+} catch (e) {
+  assert(e.message === "foo");
+  assert(e instanceof ReferenceError);
+}
+
+// Checking behavior when no length property defined
+var obj = { pop : Array.prototype.pop };
+assert (obj.length === undefined);
+assert (obj.pop() === undefined);
+assert (obj.length === 0);
+
+// Checking behavior when unable to get element
+var obj = { pop : Array.prototype.pop, length : 1};
+Object.defineProperty(obj, '0', { 'get' : referenceErrorThrower } );
+
+try {
+  obj.pop();
+  assert(false);
+} catch (e) {
+  assert(e.message === "foo");
+  assert(e instanceof ReferenceError);
+}

[galpeter]

Hello, @ruben-ayrapetyan

Regarding the ECMA_TRY_CATCH macro usage: I think that it clutters the code in more complex situations. Like if there is a for iteration in the code (just like in the join method), then to correctly free everything with the ECMA_FINALIZE, the developer needs to add a break into the for, and extra checks outside of it to ensure the for loop ran fine. Also having early returns with the !ecma_is_completion_value_normal(..) calls makes the reading of the code a top to down flow, does not requires the knowledge behind the ECMA_TRY_CATCH macro, thus making it more easier for new developers to read/write/update the code.

Additional note:
In your updated code there was a JERRY_ASSERT (len > 0), that's not needed as it is always true in that case, as the len is an uint32_t number.

[dbatyai]

Hi Ruben!

I've updated the patch based on what you supplied, and will revisit my other patches as well.

[ruben-ayrapetyan]

Hello, @galpeter

I agree that ECMA_TRY_CATCH is not the best possible solution, but it is the current practice, with already existing development line and ideas behind it.At the proper time, maybe we would rework the practice. But if we change the practice, we should do it purposefully, not only in some engine parts.

As for JERRY_ASSERT, assertions are required to be always true. That is they're direct appointment to check that condition is true.

Specifically, the JERRY_ASSERT (len > 0); assertion:

• corresponds to statement 15.4.4.6.5 of ECMA, so following the pseudocode;
• once more shows what the if block handles.

[ruben-ayrapetyan]

@dbatyai ,
there are still no comments to some function's arguments.

Also, cppcheck warnings that initialization of ret_value is not necessary on line 56, as the variable is written again later, before being read.

From other view points, your patch seems to be ready for merge.

Please, fix cppcheck's warning, add the comments, make sure that precommit passes successfully (make precommit -j) and rebase pull request up to current master.

[dbatyai]

@ruben-ayrapetyan,
I fixed the remaining issues, and rebased to current master.