Implement ES2015 class feature (part II.) by rerobika · Pull Request #2439 · jerryscript-project/jerryscript

rerobika · 2018-07-30T13:23:39Z

This patch is the second milestone of the implementation of this new language element.

Supported:

Single class inheritance
Functionality of 'super' keyword
Implicit constructor in class heritage
Specific behaviour while extending with the built-in 'Array' or '%TypedArray%' object
Abstract subclasses (Mix-ins)

JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu

zherczeg · 2018-08-02T06:50:42Z

jerry-core/ecma/base/ecma-helpers.h

 ecma_object_t *ecma_create_decl_lex_env (ecma_object_t *outer_lexical_environment_p);
-ecma_object_t *ecma_create_object_lex_env (ecma_object_t *outer_lexical_environment_p, ecma_object_t *binding_obj_p);
+ecma_object_t *ecma_create_object_lex_env (ecma_object_t *outer_lexical_environment_p,
+                                           ecma_object_t *binding_obj_p,


You can keep this in the previous line.

zherczeg · 2018-08-02T06:51:54Z

jerry-core/ecma/builtin-objects/ecma-builtin-global.c

  }

+#ifndef CONFIG_DISABLE_ES2015_CLASS
+  uint32_t super_call_opts = JERRY_CONTEXT (vm_top_context_p)->super_call_opts;


This could be a simple global variable. Maybe combined with a direct_eval flag.

jerry-core/ecma/operations/ecma-array-object.h

zherczeg · 2018-08-02T06:56:08Z

jerry-core/vm/vm.c

+#ifndef CONFIG_DISABLE_ES2015_CLASS
+    if (JERRY_UNLIKELY (frame_ctx_p->super_call_opts & ECMA_SUPER_CALL_CONSTRUCTOR))
+    {
+      stack_top_p = (ecma_value_t *) ecma_op_function_set_construct_flag (stack_top_p);


I would store these in local variables rather than manipulating globals. Just like is_direct_eval above.

zherczeg · 2018-08-02T06:57:31Z

jerry-core/ecma/operations/ecma-lex-env.c

  {
-    JERRY_ASSERT (ecma_get_lex_env_type (lex_env_p) == ECMA_LEXICAL_ENVIRONMENT_THIS_OBJECT_BOUND);
+#ifndef CONFIG_DISABLE_ES2015_CLASS
+    JERRY_ASSERT (lex_env_type != ECMA_LEXICAL_ENVIRONMENT_DECLARATIVE);


Since this is an assert, use == twice.

zherczeg · 2018-08-02T07:05:31Z

jerry-core/parser/js/js-lexer.h

 #define JS_LEXER_H

+#include "lit-char-helpers.h"
+


I would rather not include this here.

zherczeg · 2018-08-02T07:08:12Z

jerry-core/parser/js/js-parser-expr.c

+    if (context_p->status_flags & PARSER_IS_ARROW_FUNCTION)
+    {
+      status_flags |= PARSER_IS_ARROW_FUNCTION;
+    }


Ok, you can do this in a less complicated way:

const uint32_t inherited_flags = PARSER_CLASS_HAS_SUPER | PARSER_IS_ARROW_FUNCTION; status_flags |= (context_p->status_flags & inherited_flags);

Why arrow function is inherited?

zherczeg · 2018-08-02T07:11:06Z

jerry-core/parser/js/js-parser-expr.c

+    parser_parse_super_class_context_start (context_p);
+  }
+
  /* Currently heritage is not supported so the next token must be left brace. */


Is this comment still valid?

zherczeg · 2018-08-02T07:12:12Z

jerry-core/parser/js/js-parser-expr.c

+#else /* CONFIG_DISABLE_ES2015_CLASS */
      parser_emit_cbc (context_p, CBC_PUSH_THIS);
+#endif /* !CONFIG_DISABLE_ES2015_CLASS */
+


No need this newline.

zherczeg · 2018-08-02T07:17:08Z

jerry-core/parser/js/js-parser-statm.c

+        || context_p->last_cbc_opcode == CBC_PUSH_PROP_THIS_LITERAL))
+  {
+    /* Invalid LeftHandSide expression. */
+    parser_raise_error (context_p, PARSER_ERR_INVALID_LEFT_HAND_SIDE_EXPRESSION);


ES6 standard is confusing for me. It allows f() = 5 lefthandside expressions, but says it is a ReferenceError during runtime.

For classes it has meaning: class A extends f() {} is ok.

rerobika · 2018-08-21T07:22:44Z

@zherczeg Thanks for the review, I'm going to apply your suggestions.

rerobika · 2018-09-17T07:55:21Z

@zherczeg The patch is updated and ready for the review.

zherczeg · 2018-09-18T06:25:34Z

jerry-core/ecma/base/ecma-globals.h

+                                                     *   with provided super reference */

  ECMA_LEXICAL_ENVIRONMENT_TYPE_START = ECMA_LEXICAL_ENVIRONMENT_DECLARATIVE, /**< first lexical
                                                                               *    environment type */


Delete one space before comment.

jerry-core/ecma/base/ecma-helpers.c

zherczeg · 2018-09-18T06:47:23Z

jerry-core/ecma/base/ecma-helpers.c

  JERRY_ASSERT (object_p != NULL);
  JERRY_ASSERT (ecma_is_lexical_environment (object_p));
-  JERRY_ASSERT (ecma_get_lex_env_type (object_p) == ECMA_LEXICAL_ENVIRONMENT_THIS_OBJECT_BOUND);
+  JERRY_ASSERT (ecma_get_lex_env_type (object_p) != ECMA_LEXICAL_ENVIRONMENT_DECLARATIVE);


Don't use != in assert.

zherczeg · 2018-09-18T06:51:58Z

jerry-core/ecma/operations/ecma-array-object.c

+  ecma_object_t *constructor_object_p = ecma_get_object_from_value (constructor_value);
+
+  ecma_value_t result = ecma_op_create_array_object (arguments_list_p,
+                                                    arguments_list_len,


Add a space.

jerry-core/ecma/operations/ecma-array-object.c

zherczeg · 2018-09-18T07:37:05Z

jerry-core/ecma/operations/ecma-function-object.c

+ * @return the bytecode of the most general 'super'.
+ */
+static const ecma_compiled_code_t *
+ecma_resolve_implicit_constructor_call (const ecma_compiled_code_t *bytecode_data_p, /**< byte-code data header */


Lets try to use bound functions.

zherczeg · 2018-09-18T08:09:02Z

jerry-core/ecma/operations/ecma-reference.c

  return NULL;
 } /* ecma_op_resolve_reference_base */

+


Remove this newline.

zherczeg · 2018-09-18T08:18:55Z

jerry-core/parser/js/js-parser-expr.c

+              eval_parse_opts |= ECMA_PARSE_ARROW_FUNCTION;
+            }
+
+


One newline.

jerry-core/parser/js/js-parser-statm.c

zherczeg · 2018-09-18T08:46:27Z

jerry-core/vm/vm.c

+    {
+      this_value = frame_ctx_p->this_binding;
+      frame_ctx_p->super_call_opts &= (uint8_t) ~ECMA_HERITAGE_SUPER_PROP_CALL;
+    }


Do this in the parser.

zherczeg · 2018-09-25T06:33:12Z

jerry-core/ecma/base/ecma-globals.h

                                               *   ecma_op_object_find */
  ECMA_VALUE_REGISTER_REF = ECMA_MAKE_VALUE (8), /**< register reference,
                                                  *   a special "base" value for vm */
+  ECMA_VALUE_IMPLICIT_CALL = ECMA_MAKE_VALUE (9), /**< TODO */


zherczeg · 2018-09-25T06:34:30Z

jerry-core/ecma/base/ecma-globals.h

+  ECMA_PARSE_DIRECT_EVAL = (1 << 1), /**< is eval called directly (ECMA-262 v5, 15.1.2.1.1) */
+  ECMA_PARSE_HAS_SUPER = (1 << 2), /**< the current context has super reference */
+  ECMA_PARSE_CLASS_CONSTRUCTOR = (1 << 3), /**< the current context is a class constructor */
+  ECMA_PARSE_ARROW_FUNCTION = (1 << 4), /**< the current context is an arrow function */


Do we really need this flag?

zherczeg · 2018-09-25T06:39:04Z

jerry-core/ecma/base/ecma-globals.h

+typedef enum
+{
+  ECMA_HERITAGE_NO_OPTS = 0, /**< no speficied options are provided */
+  ECMA_HERITAGE_SUPER_CALLED = (1 << 1), /**< 'super ()' has been called in the current context */


What about using ECMA_OBJECT_FLAG_EXTENSIBLE?

jerry-core/ecma/base/ecma-globals.h

zherczeg · 2018-09-25T06:48:39Z

jerry-core/ecma/operations/ecma-array-object.c

+                                                     is_treat_single_arg_as_length);
+
+  ecma_object_t *result_object_p = ecma_get_object_from_value (result);
+


Less newline please.

zherczeg · 2018-09-25T08:44:01Z

jerry-core/vm/vm.c

+  JERRY_ASSERT (frame_ctx_p->call_operation == VM_EXEC_SUPER_CALL);
+
+  /* This flag is only used to force the call operation to VM_EXEC_SUPER_CALL
+    so it is no longer needed. */


Remove this comment.

jerry-core/vm/vm.c

zherczeg · 2018-09-25T08:47:55Z

jerry-core/vm/vm.c

+            goto error;
+          }
+
+          *stack_top_p++ = ecma_copy_value (ecma_op_get_super_this_binding (frame_ctx_p->lex_env_p));


class this binding

zherczeg · 2018-09-25T08:51:10Z

jerry-core/vm/vm.c

+          ecma_extended_object_t *current_ext_func_obj_p = (ecma_extended_object_t *) current_constructor_obj_p;
+
+          current_constructor_obj_p->type_flags_refs &= (uint16_t) ~ECMA_OBJECT_TYPE_MASK;
+          current_constructor_obj_p->type_flags_refs |= ECMA_OBJECT_TYPE_FUNCTION;


+= ECMA_OBJECT_TYPE_FUNCTION - ECMA_OBJECT_TYPE_EXTERNAL_FUNCTION

zherczeg · 2018-09-25T08:51:57Z

jerry-core/vm/vm.c

+        }
+        case VM_OC_SUPER_PROP_REFERENCE:
+        {
+          JERRY_ASSERT (byte_code_start_p[0] == CBC_EXT_OPCODE);


Remove this.

zherczeg · 2018-09-28T12:10:32Z

jerry-core/ecma/base/ecma-globals.h


 /**
 * Option flags for script parsing.
+ * NOTE: The enum members must be kept int sync with parser_general_flags_t


zherczeg · 2018-09-28T12:12:12Z

jerry-core/ecma/base/ecma-globals.h

+  ECMA_PARSE_DIRECT_EVAL = (1u << 1), /**< is eval called directly (ECMA-262 v5, 15.1.2.1.1) */
+  ECMA_PARSE_CLASS_CONSTRUCTOR = (1u << 2), /**< the current context is a class constructor */
+  ECMA_PARSE_HAS_SUPER = (1u << 3), /**< the current context has super reference */
+  ECMA_PARSE_CLASS_STATIC_FUNCTION = (1u << 4), /**< the current context is a static class method */


ECMA_PARSE_HAS_STATIC_SUPER

zherczeg · 2018-09-28T12:12:57Z

jerry-core/ecma/base/ecma-globals.h

-  ECMA_PARSE_DIRECT_EVAL = (1 << 1) /**< is eval called directly (ECMA-262 v5, 15.1.2.1.1) */
+  ECMA_PARSE_STRICT_MODE = (1u << 0), /**< enable strict mode */
+  ECMA_PARSE_DIRECT_EVAL = (1u << 1), /**< is eval called directly (ECMA-262 v5, 15.1.2.1.1) */
+  ECMA_PARSE_CLASS_CONSTRUCTOR = (1u << 2), /**< the current context is a class constructor */


a class constructor is being parsed.

zherczeg · 2018-09-28T12:14:42Z

jerry-core/ecma/base/ecma-globals.h

+/**
+ * Set JERRY_CONTEXT (status_flags) top 8 bits to the specified 'opts'.
+ */
+#define ECMA_SET_SUPER_EVAL_PARSER_OPTS(base, opts) \


I think "base" is unnecessary.

zherczeg · 2018-09-28T12:18:44Z

jerry-core/ecma/operations/ecma-array-object.c

+ */
+ecma_value_t
+ecma_op_create_array_object_by_constructor (const ecma_value_t *arguments_list_p, /**< list of arguments that
+                                                                                   * are passed to Array constructor */


indentation is missing.

zherczeg · 2018-09-28T13:08:20Z

jerry-core/ecma/operations/ecma-function-object.c

+ecma_op_set_class_context (ecma_object_t *local_env_p, /**< local lexical enviroment */
+                           ecma_value_t this_binding) /**< 'this' argument's value */
+{
+  ecma_string_t *super_this_binding_string_p = ecma_get_magic_string (LIT_INTERNAL_MAGIC_STRING_CLASS_THIS_BINDING);


class_this_binding

zherczeg · 2018-10-01T08:08:07Z

jerry-core/parser/js/byte-code.h

+  /* Class opcodes */ \
+  CBC_OPCODE (CBC_EXT_INHERIT_AND_SET_CONSTRUCTOR, CBC_NO_FLAG, 0, \
+              VM_OC_CLASS_HERITAGE) \
+  CBC_OPCODE (CBC_EXT_SUPER_EVAL, CBC_HAS_BYTE_ARG, 0, \


CBC_EXT_CLASS_EVAL

zherczeg · 2018-10-01T08:08:50Z

jerry-core/parser/js/byte-code.h

+              VM_OC_SUPER_CALL | VM_OC_PUT_STACK) \
+  CBC_OPCODE (CBC_EXT_SUPER_CALL_BLOCK, CBC_HAS_POP_STACK_BYTE_ARG, -1, \
+              VM_OC_SUPER_CALL | VM_OC_PUT_BLOCK) \
+  CBC_OPCODE (CBC_EXT_PUSH_CLASS_CONSTRUCTOR, CBC_NO_FLAG, 1, \


move after CBC_EXT_INHERIT_AND_SET_CONSTRUCTOR

jerry-core/parser/js/js-parser.c

zherczeg · 2018-10-01T08:28:06Z

jerry-core/vm/vm.c

+            ecma_value_t super_prototype_value = ecma_op_object_get_by_magic_id (super_class_p,
+                                                                                 LIT_MAGIC_STRING_PROTOTYPE);
+
+            if (!ECMA_IS_VALUE_ERROR (super_prototype_value) && !ecma_is_value_undefined (super_prototype_value))


If not object or null, throw an error.

rerobika · 2018-10-02T13:21:29Z

The patch is updated, depends on #2547.

zherczeg · 2018-10-04T11:05:04Z

jerry-core/ecma/base/ecma-globals.h

  ECMA_TYPE_ERROR = 7, /**< pointer to description of an error reference (only supported by C API) */
  ECMA_TYPE_POINTER = ECMA_TYPE_ERROR, /**< a generic aligned pointer */
+#ifndef CONFIG_DISABLE_ES2015_CLASS
+  ECMA_TYPE_CLASS = ECMA_TYPE_ERROR, /**< special type for class contextes */


ECMA_TYPE_INACCESSIBLE_THIS

zherczeg · 2018-10-04T11:06:46Z

jerry-core/ecma/base/ecma-globals.h

                                               *   ecma_op_object_find */
  ECMA_VALUE_REGISTER_REF = ECMA_MAKE_VALUE (8), /**< register reference,
                                                  *   a special "base" value for vm */
+  ECMA_VALUE_IMPLICIT_CALL = ECMA_MAKE_VALUE (9), /**< special value for implicit constructor call */


comment: bound class constructors
ECMA_VALUE_IMPLICIT_CONSTRUCTOR

zherczeg · 2018-10-04T11:33:53Z

jerry-core/ecma/operations/ecma-function-object.c

+
+  if (ecma_is_value_class (this_binding))
+  {
+    this_binding -= ECMA_TYPE_CLASS - ECMA_TYPE_OBJECT;;


Double semicolon.

jerry-core/ecma/operations/ecma-function-object.c

zherczeg · 2018-10-04T11:47:38Z

jerry-core/ecma/operations/ecma-function-object.c


  /* 8. */
-  ecma_value_t ret_value;
+  ecma_value_t ret_value = ECMA_VALUE_UNDEFINED;


Move this before the break statement.

zherczeg · 2018-10-04T11:56:22Z

jerry-core/lit/lit-magic-strings.h

  LIT_INTERNAL_MAGIC_STRING_PROMISE_PROPERTY_INDEX, /**< [[Index]] property */
  LIT_INTERNAL_MAGIC_STRING_PROMISE_PROPERTY_VALUE, /**< [[Values]] property */
  LIT_INTERNAL_MAGIC_STRING_PROMISE_PROPERTY_REMAINING_ELEMENT, /**< [[RemainingElement]] property */
+  LIT_INTERNAL_MAGIC_STRING_CLASS_THIS_BINDING, /**< the this binding of the class constructor */


Please use internal properties.

jerry-core/parser/js/byte-code.h

zherczeg · 2018-10-04T12:08:53Z

jerry-core/parser/js/js-parser-internal.h

+/**
+ * Offset between PARSER_CLASS_CONSTRUCTOR and ECMA_PARSE_CLASS_CONSTRUCTOR
+ */
+#define PARSER_CLASS_PARSE_OPTS_OFFSET 18


zherczeg · 2018-10-04T12:09:21Z

jerry-core/parser/js/js-parser-internal.h

@@ -69,6 +69,9 @@ typedef enum
 #endif /* !CONFIG_DISABLE_ES2015_ARROW_FUNCTION */
 #ifndef CONFIG_DISABLE_ES2015_CLASS
  PARSER_CLASS_CONSTRUCTOR = (1u << 20),      /**< a class constructor is parsed */


Add a comment about PARSER_CLASS_PARSE_OPTS_OFFSET

zherczeg

Getting very close.

zherczeg · 2018-10-08T08:38:30Z

jerry-core/ecma/base/ecma-globals.h

-  ECMA_PARSE_STRICT_MODE = (1 << 0), /**< enable strict mode */
-  ECMA_PARSE_DIRECT_EVAL = (1 << 1) /**< is eval called directly (ECMA-262 v5, 15.1.2.1.1) */
+  ECMA_PARSE_STRICT_MODE = (1u << 0), /**< enable strict mode */
+  ECMA_PARSE_DIRECT_EVAL = (1u << 1), /**< is eval called directly (ECMA-262 v5, 15.1.2.1.1) */


comment: eval is called directly

jerry-core/ecma/operations/ecma-array-object.c

zherczeg · 2018-10-08T08:47:46Z

jerry-core/ecma/operations/ecma-array-object.c


+#ifndef CONFIG_DISABLE_ES2015_CLASS
+/**
+ * Array object creation operation according to the 'constructor' property.


comment: Array object creation with custom prototype.

jerry-core/ecma/operations/ecma-function-object.c

zherczeg · 2018-10-08T08:58:55Z

jerry-core/ecma/operations/ecma-function-object.c

+    case ECMA_OBJECT_TYPE_BOUND_FUNCTION:
+    {
+      JERRY_ASSERT (!ecma_op_function_has_construct_flag (arguments_list_p));
+      ret_value = ecma_op_function_construct (func_obj_p,


I would pass the target_function here. Requires one less check after the recursive call.

zherczeg · 2018-10-08T10:03:37Z

jerry-core/parser/js/js-parser-statm.c

+        uint32_t constructor_super = PARSER_CLASS_CONSTRUCTOR | PARSER_CLASS_HAS_SUPER;
+        bool is_constructor_super = (context_p->status_flags & constructor_super) == constructor_super;
+#else /* CONFIG_DISABLE_ES2015_CLASS */
+        bool is_constructor_super = false;


Could we do this nicely without introducing a variable?

zherczeg · 2018-10-08T10:05:08Z

jerry-core/parser/js/js-parser.c

                 && (status_flags & PARSER_IS_ARROW_FUNCTION));
  parser_save_context (context_p, &saved_context);
+#ifndef CONFIG_DISABLE_ES2015_CLASS
+  context_p->status_flags |= status_flags | PARSER_ARGUMENTS_NOT_NEEDED | class_has_super;


Remove the uint32_t class_has_super = context_p->status_flags & PARSER_CLASS_HAS_SUPER; above.

Add context_p->status_flags |= context_p->status_flags & PARSER_CLASS_HAS_SUPER here if the line above is too long with this.

If you save the context you lose all of your status flags except the PARSER_IS_STRICT, so
context_p->status_flags |= context_p->status_flags & PARSER_CLASS_HAS_SUPER will never add the PARSER_CLASS_HAS_SUPER flag to the new context.

But we can combine our solutions like:

context_p->status_flags |= saved_context.status_flags & PARSER_CLASS_HAS_SUPER

zherczeg · 2018-10-08T10:10:16Z

jerry-core/vm/vm.c

+
+                ecma_free_value (result);
+
+                result = ecma_raise_type_error ("Class extends value is not a constructor or null.");


Value provided by class extends is not an object or null.

zherczeg · 2018-10-08T10:11:43Z

jerry-core/vm/vm.c

+
+            frame_ctx_p->lex_env_p = super_env_p;
+          }
+          else


It seems the two code paths do not share code. Would it be a good idea to introduce two vm opcodes for them?

zherczeg · 2018-10-08T10:15:51Z

jerry-core/vm/vm.c

            ecma_fast_free_value (block_result);
            block_result = result;
          }
+


Why this space is needed?

akosthekiss

First set of comments. More may come, I couldn't review all parts of the change yet.

akosthekiss · 2018-10-08T14:31:31Z

jerry-core/ecma/base/ecma-globals.h


 /**
 * Option flags for script parsing.
+ * NOTE: The enum members must be kept in sync with parser_general_flags_t


AFAIK, notes are written in a slightly different way in the rest of the code base:

* * Note: * text here

jerry-core/ecma/base/ecma-globals.h

akosthekiss · 2018-10-08T14:37:07Z

jerry-core/jcontext/jcontext.h

  uint32_t lit_magic_string_ex_count; /**< external magic strings count */
  uint32_t jerry_init_flags; /**< run-time configuration flags */
-  uint32_t status_flags; /**< run-time flags */
+  uint32_t status_flags; /**< run-time flags. NOTE: the top 8 bits are used for passing class parsing options */


That all-caps note is very 'loud'. I'd suggest simply "run-time flags (the top 8 bits are used for passing class parsing options)"

akosthekiss · 2018-10-08T14:43:54Z

jerry-core/lit/lit-magic-strings.h

+ * Check whether the current magic string is internal magic string
+ */
+#define LIT_IS_INTERNAL_MAGIC_STRING(id) ((id) >= LIT_GC_MARK_REQUIRED_MAGIC_STRING__COUNT \
+                                          && (id) < LIT_MAGIC_STRING__COUNT)


This second part of the predicate seems to be a safety check that usually goes into an assert to prevent being a performance hit. Cannot LIT_IS_INTERNAL_MAGIC_STRING only check for (id) >= LIT_GC_MARK_REQUIRED_MAGIC_STRING__COUNT? Callers of LIT_IS_INTERNAL_MAGIC_STRING can have a separate assert for (id) < LIT_MAGIC_STRING__COUNT, which will then be a no-op for release builds (or LIT_IS_INTERNAL_MAGIC_STRING can have a different version for debug/release builds, but I think there is no precedent for that in the code base).

I think this is not used by asserts. Live code should be fast.

@akosthekiss Your insight is right about the safety check. The reason of this plus check is that this macro is only used in asserts.

case ECMA_PROPERTY_TYPE_INTERNAL: { JERRY_ASSERT (ECMA_PROPERTY_GET_NAME_TYPE (property) == ECMA_DIRECT_STRING_MAGIC && LIT_IS_INTERNAL_MAGIC_STRING (property_pair_p->names_cp[index])); break; }

default: { JERRY_ASSERT (ECMA_PROPERTY_GET_TYPE (*property_p) == ECMA_PROPERTY_TYPE_INTERNAL); /* Must be a native pointer. */ JERRY_ASSERT (ECMA_PROPERTY_GET_NAME_TYPE (*property_p) == ECMA_DIRECT_STRING_MAGIC && LIT_IS_INTERNAL_MAGIC_STRING (name_cp)); break; }

Anyway the property's type is the most significant attribute during the property processing we can not rely on this macro, it's only for safety check.
Therefore I think it's not necessary to modify this macro to support release builds.

If this is only used twice and both times in an assert, is a macro (that can be misused by mistake and thus become a performance hit) justified for this? Can't this check simply be incorporated into the two asserts?

Sure, you convinced me.

jerry-core/parser/js/byte-code.h

akosthekiss · 2018-10-08T14:58:15Z

jerry-core/parser/js/js-parser-expr.c


+#ifndef CONFIG_DISABLE_ES2015_CLASS
+#include "lit-char-helpers.h"
+#endif /* !CONFIG_DISABLE_ES2015_CLASS */


Note 1: lit-char-helpers.h is already included some lines above, guarded by CONFIG_DISABLE_ES2015_TEMPLATE_STRINGS. These could be merged.

Note 2: But what's the point of conditional includes? What harm can the unconditional includes do? I see that other header inclusions are guarded, too, but don't really see why. I'd just write

#include "jcontext.h" #include "lit-char-helpers.h"

If this causes any problems in this compilation unit in some configuration, then that signals quite a problem somewhere.

I followed the #ifdef guard terminology, but +1 for removing them.

akosthekiss · 2018-10-08T15:07:02Z

jerry-core/parser/js/js-parser-internal.h

 #ifndef CONFIG_DISABLE_ES2015_CLASS
-  PARSER_CLASS_CONSTRUCTOR = (1u << 20),      /**< a class constructor is parsed */
+  PARSER_CLASS_CONSTRUCTOR = (1u << 20),      /**< a class constructor is parsed. NOTE: this value must be kept in
+                                               *   in sync with ECMA_PARSE_CLASS_CONSTRUCTOR */


Is it only this enumerator that must be kept in sync with another enumerator? The newly added note at ecma_parse_opts_t mentions all enumerators in this enum. BTW, what does "in sync" mean in this case? See:

ECMA_PARSE_CLASS_CONSTRUCTOR = (1u << 2), /**< a class constructor is being parsed */ ECMA_PARSE_HAS_SUPER = (1u << 3), /**< the current context has super reference */ ECMA_PARSE_HAS_STATIC_SUPER = (1u << 4), /**< the current context is a static class method */

vs

PARSER_CLASS_CONSTRUCTOR = (1u << 20), /**< a class constructor is parsed */ PARSER_CLASS_HAS_SUPER = (1u << 21) /**< class has super reference */ PARSER_CLASS_STATIC_FUNCTION = (1u << 22), /**< this function is a static class method */

The values are definitely not the same, and the enumerator names are also a bit 'out-of-sync'.

I've just found PARSER_CLASS_PARSE_OPTS_OFFSET. It should be referenced in the comment (or perhaps in a comment inserted before PARSER_CLASS_CONSTRUCTOR to emphasise that the following enumerators form a group and have to be treated together).

Static asserts could be used to check this.

The reference in the comment is a good idea, static assert for the check is already available.

akosthekiss · 2018-10-08T15:08:09Z

jerry-core/parser/js/js-parser-statm.c

@@ -21,6 +21,8 @@
 #include "jcontext.h"
 #endif /* JERRY_DEBUGGER || JERRY_ENABLE_LINE_INFO */


(Also a conditional include. Not part of this PR but still not sure why.)

akosthekiss · 2018-10-08T15:19:29Z

jerry-core/vm/vm-stack.c

      ecma_deref_object (lex_env_p);

+#ifndef CONFIG_DISABLE_ES2015_CLASS
+      JERRY_ASSERT (PARSER_WITH_CONTEXT_STACK_ALLOCATION == PARSER_SUPER_CLASS_CONTEXT_STACK_ALLOCATION);


Shouldn't this rather be a static assert? Both PARSER_WITH_CONTEXT_STACK_ALLOCATION and PARSER_SUPER_CLASS_CONTEXT_STACK_ALLOCATION are macro-defined constants in byte-code.h. (And PARSER_SUPER_CLASS_CONTEXT_STACK_ALLOCATION is not even macro-guarded. So either the (static?) assert should not be guarded or PARSER_SUPER_CLASS_CONTEXT_STACK_ALLOCATION should be macro-guarded.)

Sure, I can agree with that.

jerry-core/parser/js/js-parser-statm.c

akosthekiss

Second set of comments. I got 20 of the 30 files covered now.

akosthekiss · 2018-10-08T15:45:43Z

jerry-core/ecma/base/ecma-helpers.h

 bool JERRY_ATTR_CONST ecma_is_value_found (ecma_value_t value);
 bool JERRY_ATTR_CONST ecma_is_value_array_hole (ecma_value_t value);
+#ifndef CONFIG_DISABLE_ES2015_CLASS
+bool JERRY_ATTR_CONST ecma_is_value_class (ecma_value_t value);


I cannot find this function either implemented or used anywhere.

Good catch, it's a leftover code!

akosthekiss · 2018-10-08T15:46:54Z

jerry-core/ecma/operations/ecma-eval.c

+#if defined (JERRY_ENABLE_LINE_INFO) || !defined (CONFIG_DISABLE_ES2015_CLASS)
 #include "jcontext.h"
-#endif /* JERRY_ENABLE_LINE_INFO */
+#endif /* defined (JERRY_ENABLE_LINE_INFO) || !defined (CONFIG_DISABLE_ES2015_CLASS) */


Yet another conditional include. I still suggest dropping the guards.

akosthekiss · 2018-10-08T16:09:50Z

jerry-core/parser/js/js-parser.c

 #ifndef JERRY_DISABLE_JS_PARSER

+JERRY_STATIC_ASSERT ((int) ECMA_PARSE_STRICT_MODE == (int) PARSER_IS_STRICT,
+                     ecma_parse_strict_mode_and_parser_is_strict_are_equal);


AFAIK the wording of the static assert 'messages' should contain "must", "must be", "should be", "must not", etc. That's because "xxx_and_yyy_are_equal" is somewhat misleading. If the assert is triggered and this text is printed, it's not really clear what the cause of the error is. (The message could suggest that they are equal, while the real problem is that they aren't.)

I see, I'll rename it.

akosthekiss · 2018-10-08T16:10:20Z

jerry-core/parser/js/js-parser.c

+
+#ifndef CONFIG_DISABLE_ES2015_CLASS
+JERRY_STATIC_ASSERT ((ECMA_PARSE_CLASS_CONSTRUCTOR << PARSER_CLASS_PARSE_OPTS_OFFSET) == PARSER_CLASS_CONSTRUCTOR,
+                     ecma_class_parse_options_can_be_shifted_to_ecma_general_flags);


Same static assert message problem here.

jerry-core/parser/js/js-parser-expr.c

akosthekiss · 2018-10-08T16:53:42Z

tests/jerry/es2015/class-inheritance.js

+must_throw ("class B extends A { constructor (a, b) { this.b = b} } \
+             var b = new B (1,2);");
+
+must_throw ("class B extends A { constructor (a,b) { super.f() } } \


Args should be comma+space-separated. That one extra space is not a big price for readability. Also when calling a function (above, new B (1,2)). Throughout this file.

akosthekiss · 2018-10-08T16:55:14Z

tests/jerry/es2015/class-inheritance.js

+}
+
+class B extends A {
+


Is this empty line needed?

akosthekiss · 2018-10-08T17:03:17Z

tests/jerry/es2015/class-inheritance.js

+}
+
+var d = new D;
+assert (Object.getPrototypeOf(d) == D.prototype);


In general, this test file is huge. The problem with that is that it's full with asserts and if this test fails, it becomes hard to find which part is problematic. I think it should be possible to split up this file into smaller ones that test fewer functionalities.

How about splitting the tests file into:

class-inheritance-early-semantics.js (must throw part)

class-inheritance-main.js (core functionalities)

class-inheritance-builtin-extends.js(specific behaviour with Array / %Typedarray%)

class-inheritance-mixins.js (Mix-ins + bound functions)

(Also with the unified style)

Sounds good as a start.

akosthekiss · 2018-10-08T17:10:41Z

jerry-core/parser/js/js-parser-internal.h

 #ifndef CONFIG_DISABLE_ES2015_CLASS
-  PARSER_CLASS_CONSTRUCTOR = (1u << 20),      /**< a class constructor is parsed */
+  PARSER_CLASS_CONSTRUCTOR = (1u << 20),      /**< a class constructor is parsed. NOTE: this value must be kept in
+                                               *   in sync with ECMA_PARSE_CLASS_CONSTRUCTOR */


I've just found PARSER_CLASS_PARSE_OPTS_OFFSET. It should be referenced in the comment (or perhaps in a comment inserted before PARSER_CLASS_CONSTRUCTOR to emphasise that the following enumerators form a group and have to be treated together).

akosthekiss · 2018-10-08T17:14:19Z

jerry-core/parser/js/js-parser-internal.h

+/**
+ * Mask for get class option bits from ecma_parse_opts_t
+ */
+#define PARSER_CLASS_ECMA_PARSE_OPTS_TO_PARSER_OPTS_MASK 0x1C


There should be a way to specify a static assert for this, otherwise this will become a fragile hard-coded constant.

Still thinking on a proper static assert for this...

akosthekiss

Third set of comments. Got through all files now.

akosthekiss · 2018-10-08T17:32:31Z

jerry-core/ecma/operations/ecma-array-object.c

 #include "ecma-objects-general.h"
+#ifndef CONFIG_DISABLE_ES2015_CLASS
+#include "ecma-function-object.h"
+#endif /* !CONFIG_DISABLE_ES2015_CLASS */


Conditional include, guards to be dropped.

jerry-core/ecma/operations/ecma-typedarray-object.c

akosthekiss · 2018-10-08T18:25:25Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+
+  ecma_deref_object (proto_p);
+
+  ecma_object_t *constructor_prototpye_object_p = ecma_get_object_from_value (constructor_prototype);


typo: constructor_prototpye_object_p -> constructor_prototype_object_p

Thanks, I didn't notice it.

akosthekiss · 2018-10-08T18:43:17Z

jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.c

+    return new_array;
+  }
+#else /* CONFIG_DISABLE_ES2015_CLASS */
  ecma_value_t new_array = ecma_op_create_array_object (0, 0, false);


It's interesting that the old code never checks for error although ecma_op_create_array_object may raise errors. But only if length was invalid, which is known not to happen for the hard-coded params. A JERRY_ASSERT (!ECMA_IS_VALUE_ERROR (new_array)); might still have its place here to signal that.

Good idea, an assert never hurts.

akosthekiss · 2018-10-08T18:44:59Z

jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.c

  /* 2. */
+#ifndef CONFIG_DISABLE_ES2015_CLASS
+  ecma_object_t *obj_p = ecma_get_object_from_value (obj_this);
+  ecma_value_t new_array = ecma_op_create_array_object_by_constructor (0, 0, false, obj_p);


First argument is of type ecma_value_t *arguments_list_p, so NULL would be a better fit here instead of the 0. (The old code might also be improved.)

jerry-core/parser/js/js-parser-statm.c

akosthekiss · 2018-10-08T23:57:54Z

jerry-core/parser/js/js-parser-statm.c

        {
          context_p->last_cbc_opcode = CBC_RETURN_WITH_LITERAL;
        }
+#endif /* !CONFIG_DISABLE_ES2015_CLASS */


Same as

#ifndef CONFIG_DISABLE_ES2015_CLASS if ((context_p->status_flags & constructor_super) != constructor_super) { #endif /* !CONFIG_DISABLE_ES2015_CLASS */ if (context_p->last_cbc_opcode == CBC_PUSH_LITERAL) { context_p->last_cbc_opcode = CBC_RETURN_WITH_LITERAL; } #ifndef CONFIG_DISABLE_ES2015_CLASS } #endif /* !CONFIG_DISABLE_ES2015_CLASS */

or

if (context_p->last_cbc_opcode == CBC_PUSH_LITERAL) { #ifndef CONFIG_DISABLE_ES2015_CLASS if ((context_p->status_flags & constructor_super) != constructor_super) { #endif /* !CONFIG_DISABLE_ES2015_CLASS */ context_p->last_cbc_opcode = CBC_RETURN_WITH_LITERAL; #ifndef CONFIG_DISABLE_ES2015_CLASS } #endif /* !CONFIG_DISABLE_ES2015_CLASS */ }

?

I really don't like such constructs. So hard to read.

Well, I don't like copy-paste cloning. And the above pattern makes the code more future-proof in the sense that should we ever change the config macro infrastructure from #ifdef ENABLE/#ifndef DISABLE to #if 1 (i.e., making decisions not based on whether a macro is defined but based on its value) then the above pattern can be easily recognized and simplified into and if with a simple predicate:

if (context_p->last_cbc_opcode == CBC_PUSH_LITERAL && (!CONFIG_ES2015_CLASS || (context_p->status_flags & constructor_super) != constructor_super))

I see the advantages/disadvantages of both styles. How about summoning @LaszloLango and @robertsipka to ask their opinions?

The question is:

#ifndef CONFIG_DISABLE_ES2015_XXX if (// New condition) { // New code path } else { // Old code path } #else /* CONFIG_DISABLE_ES2015_XXX */ // Old code path #endif /* !CONFIG_DISABLE_ES2015_XXX */

vs.

#ifndef CONFIG_DISABLE_ES2015_XXX if (// New condition) { // New code path } else { #endif /* !CONFIG_DISABLE_ES2015_XXX */ // Old code path #ifndef CONFIG_DISABLE_ES2015_XXX } #endif /* !CONFIG_DISABLE_ES2015_XXX */

I prefer the second one without code duplication.

The first one is easier to read, but eliminate the code duplication has priority (+1 for the second).

Thanks for the answers, I'll use the 2nd style.

akosthekiss · 2018-10-09T00:16:16Z

jerry-core/parser/js/js-parser-statm.c

        lexer_next_token (context_p);
+
+#ifndef CONFIG_DISABLE_ES2015_CLASS
+        uint32_t constructor_super = PARSER_CLASS_CONSTRUCTOR | PARSER_CLASS_HAS_SUPER;


Make this const so that it doesn't get modified by mistake, and move it before the switch so that it doesn't have to be defined multiple times (as around line 2194). Or make this something like a #define PARSER_CLASS_CONSTRUCTOR_SUPER (PARSER_CLASS_CONSTRUCTOR | PARSER_CLASS_HAS_SUPER) if the value is used multiple times, as in js-parser.c.

#define PARSER_CLASS_CONSTRUCTOR_SUPER (PARSER_CLASS_CONSTRUCTOR | PARSER_CLASS_HAS_SUPER) is a good idea.

jerry-core/parser/js/js-parser.c

rerobika · 2018-10-09T07:09:21Z

@akosthekiss Thanks for the deep review. Let me apply a part of your suggestions first (I mean the trivial ones) then talk about the rest.

rerobika · 2018-10-09T13:53:21Z

Patch is updated to see what have been resolved. Still working on it.

rerobika · 2018-10-12T09:00:23Z

@akosthekiss The patch has been updated!

jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.c

akosthekiss · 2018-10-17T23:50:23Z

jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.c

+    }
+#else /* CONFIG_DISABLE_ES2015_CLASS */
    ecma_value_t new_array = ecma_op_create_array_object (NULL, 0, false);
+#endif /* !CONFIG_DISABLE_ES2015_CLASS */


This addition is not consistent with the previous approaches: there is no assertion check for no error.

The diff does not show it but there was an already existing assert for it in the next line. Anyway, I move it up to the else/endif block.

akosthekiss · 2018-10-17T23:50:31Z

jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.c

+    }
+#else /* CONFIG_DISABLE_ES2015_CLASS */
    ecma_value_t new_array = ecma_op_create_array_object (NULL, 0, false);
+#endif /* !CONFIG_DISABLE_ES2015_CLASS */


akosthekiss · 2018-10-18T09:36:19Z

tests/jerry/es2015/class-inheritance-core.js

+    this.g = super.f;
+    this.h = this.f;
+  }
+}


Is it intentional that A and B are redefined over and over again in the same test file with slightly different content? All redefinitions seem to be potential test file splitting points.

akosthekiss · 2018-10-18T09:39:46Z

tests/jerry/es2015/class-inheritance-early-semantics.js

+must_throw ("class A extends { constructor () { super () } }");
+
+class B extends A {
+  constructor (a,b) {


add space after comma

akosthekiss · 2018-10-18T09:42:39Z

tests/jerry/es2015/class-inheritance-early-semantics.js

+  }
+}
+var b = new B (1, 2);
+assert (b.g ()()() === 5);


It seems to me that these two checks could be merged. Constructors are the same (assert can be kept), object is instantiated the same way in both cases. Only the second g () has to be renamed (e.g., to h ()), and then this assert has to be rewritten as b.h ()()() === 5. If I get it right.

True, let's merge them.

akosthekiss · 2018-10-18T09:45:01Z

tests/jerry/es2015/class-inheritance-mixins-bounds.js

+
+var b = new B ();
+assert (b.f () === 1)
+assert (b.g () === 2);


potential splitting point? (independent tests before and after)

akosthekiss · 2018-10-18T09:45:42Z

tests/jerry/es2015/class-inheritance-mixins-bounds.js

+  }
+}
+
+new C ().foo ()


potential splitting point?

jerry-core/ecma/base/ecma-helpers.c

jerry-core/ecma/operations/ecma-array-object.h

rerobika · 2018-10-18T13:35:43Z

@akosthekiss Your suggestions have been applied!

akosthekiss · 2018-10-18T15:21:05Z

tests/jerry/es2015/class-inheritance-core-14.js

+ class A {
+ 	constructor () {
+ 		this.a = 5;
+ 	}


strange indentation

akosthekiss · 2018-10-18T15:21:41Z

tests/jerry/es2015/class-inheritance-core-5.js

+ assert (b.f === 5);
+ assert (b.length === 3);
+
+ class A extends Array {


is the redefinition of A intentional here?

akosthekiss · 2018-10-18T15:23:43Z

tests/jerry/es2015/class-inheritance-mixins.js

+
+var b = new B ();
+assert (b.f () === 1)
+assert (b.g () === 2);


splitting point?

akosthekiss · 2018-10-19T06:36:30Z

tests/jerry/es2015/class-inheritance-core-2.js

+
+ assert (D.a () === 6);
+
+ class A extends Array {


seems like a leftover in this test?

Good catch! Thanks.

akosthekiss · 2018-10-19T06:41:41Z

tests/jerry/es2015/class-inheritance-bounds.js

+ * limitations under the License.
+ */
+
+var g = Array.bind (0, 1, 2, 3)


nitpicking: why is this test file named "bounds"?

The reason is that implicit class constructors are really similar to bound functions. But let's rename it only to -bound.js, hence it only includes one test case.

This patch is the second milestone of the implementation of this new language element. Supported: - Single class inheritance - Functionality of 'super' keyword - Implicit constructor in class heritage - Specific behaviour while extending with the built-in 'Array' or '%TypedArray%' object - Abstract subclasses (Mix-ins) JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu

rerobika · 2018-10-25T12:39:42Z

@akosthekiss The PR is updated as you suggested.

akosthekiss

LGTM. That was quite some work.

This patch is the second milestone of the implementation of this new language element. Supported: - Single class inheritance - Functionality of 'super' keyword - Implicit constructor in class heritage - Specific behaviour while extending with the built-in 'Array' or '%TypedArray%' object - Abstract subclasses (Mix-ins) JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu

Picked from upstream jerryscript-project/jerryscript#2439

rerobika force-pushed the es2015_class_part_II branch 2 times, most recently from 3d8f738 to 9900717 Compare August 1, 2018 14:59

zherczeg requested changes Aug 2, 2018

View reviewed changes

akosthekiss added the ES2015 Related to ES2015 features label Aug 17, 2018

rerobika force-pushed the es2015_class_part_II branch 3 times, most recently from 3d9f3fc to d985517 Compare September 17, 2018 07:50

zherczeg requested changes Sep 18, 2018

View reviewed changes

rerobika force-pushed the es2015_class_part_II branch 3 times, most recently from 7c69ea4 to 8788c2a Compare September 21, 2018 13:28

zherczeg requested changes Sep 25, 2018

View reviewed changes

rerobika force-pushed the es2015_class_part_II branch 4 times, most recently from ff1b5b2 to 0ea5c73 Compare September 27, 2018 09:54

zherczeg requested changes Oct 1, 2018

View reviewed changes

rerobika force-pushed the es2015_class_part_II branch from 0ea5c73 to 44729d4 Compare October 2, 2018 13:20

rerobika force-pushed the es2015_class_part_II branch 3 times, most recently from 3f86596 to 61be3d8 Compare October 4, 2018 10:06

zherczeg requested changes Oct 4, 2018

View reviewed changes

rerobika force-pushed the es2015_class_part_II branch 3 times, most recently from b94cce4 to b1bdb21 Compare October 5, 2018 09:54

zherczeg requested changes Oct 8, 2018

View reviewed changes

akosthekiss requested changes Oct 8, 2018

View reviewed changes

akosthekiss requested changes Oct 9, 2018

View reviewed changes

rerobika force-pushed the es2015_class_part_II branch from 8b6584b to c54f440 Compare October 9, 2018 13:49

rerobika force-pushed the es2015_class_part_II branch 4 times, most recently from f72e2f8 to 61b4070 Compare October 11, 2018 10:27

akosthekiss requested changes Oct 18, 2018

View reviewed changes

jerry-core/ecma/base/ecma-helpers.c Show resolved Hide resolved

jerry-core/ecma/base/ecma-helpers.c Show resolved Hide resolved

jerry-core/ecma/operations/ecma-array-object.h Show resolved Hide resolved

rerobika force-pushed the es2015_class_part_II branch from 61b4070 to fcebdfc Compare October 18, 2018 12:09

akosthekiss reviewed Oct 18, 2018

View reviewed changes

rerobika force-pushed the es2015_class_part_II branch from fcebdfc to a5d4211 Compare October 18, 2018 15:53

akosthekiss reviewed Oct 19, 2018

View reviewed changes

rerobika force-pushed the es2015_class_part_II branch from a5d4211 to 481f83b Compare October 25, 2018 09:18

rerobika force-pushed the es2015_class_part_II branch from 481f83b to 7eac228 Compare October 25, 2018 11:31

akosthekiss approved these changes Oct 25, 2018

View reviewed changes

akosthekiss merged commit d1860d0 into jerryscript-project:master Oct 25, 2018

legendecas added a commit to yodaos-project/ShadowNode that referenced this pull request Nov 29, 2018

jerry: implement ES2015 class feature (part II.)

15973a3

Picked from upstream jerryscript-project/jerryscript#2439

rerobika deleted the es2015_class_part_II branch February 28, 2019 11:17

		is_treat_single_arg_as_length);

		ecma_object_t *result_object_p = ecma_get_object_from_value (result);


		ecma_free_value (result);

		result = ecma_raise_type_error ("Class extends value is not a constructor or null.");

		@@ -21,6 +21,8 @@
		#include "jcontext.h"
		#endif /* JERRY_DEBUGGER \|\| JERRY_ENABLE_LINE_INFO */


		ecma_deref_object (proto_p);

		ecma_object_t *constructor_prototpye_object_p = ecma_get_object_from_value (constructor_prototype);

Conversation

rerobika commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rerobika commented Aug 21, 2018

Uh oh!

rerobika commented Sep 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rerobika commented Jul 30, 2018 •

edited

Loading