Parser style fixes after Class part II patch.

[zherczeg]

No description provided.

[LaszloLango]

Please fix the Travis failure.

[akosthekiss]

Isn't this causing extra work for the #ifndef CONFIG_DISABLE_ES2015_CLASS && context_p->status_flags & PARSER_CLASS_HAS_SUPER case? A CBC_EVAL is emitted just to be overwritten (below) with context_p->last_cbc_opcode =.

[zherczeg]

This is how CBC works in general. You start with an opcode and mutate it to others. While this could be done a bit more efficiently, I think the perf gain is negligible, since eval is not that frequent. And looks much better.

[akosthekiss]

Yes, bytecode rewrite happens, when a different step realizes that it can do something more efficiently than the previous step. But this is different as CBC rewrite happens right after the CBC is written. I don't think that we should write intentionally inefficient code only to avoid an #ifndef.

[zherczeg]

What I don't like in the original code that it writes something manually, which is ugly and no other code do this. From performance perspective this is really a minor thing.

[akosthekiss]

I you mean by "manually" that it writes last_cbc_opcode and last_cbc.value directly, then that's still happening. To avoid that, the proper approach would be to add a new parser_emit_cbc_class_eval (or something like that) to js-parser-util.c and call that from the if (context_p->status_flags & PARSER_CLASS_HAS_SUPER) branch (and keep calling parser_emit_cbc (context_p, CBC_EVAL); otherwise).

[zherczeg]

Code rewritten. I don't like these "Outdated" labels.

[LaszloLango]

LGTM after the misaligned comment is fixed.

[LaszloLango]

@zherczeg Please rebase to the current master.

[zherczeg]

Please check the patch again.

[akosthekiss]

Please, add these back. It helps in various cases to have comma-terminated arrays (and enums). If an array (or an enum) has to me changed (new element gets added, an old one gets removed) then only that line needs to be changed that contains the affected element. One doesn't have to check whether it is/was/becomes before/after/the last element in the array (or enum) and doesn't have to add/remove another comma. It also helps if content is ifdef-guarded: again, one doesn't have to care about which element is the last. So, please keep also the last entry comma-terminated.

[zherczeg]

I think such change is rare because the items are in alphabetic order. And we usually don't put a comma after the last item in other enums as well.

[akosthekiss]

These are the arrays and enums where the use-trailing-comma-everywhere principle is used:

• arrays:
  jerry_typedarray_mappings, ecma_builtin_routines, ecma_builtin_call_functions, ecma_builtin_construct_functions, ecma_builtin_property_list_references, ecma_error_mappings, cbc_flags, cbc_ext_flags, cbc_names, cbc_ext_names, statement_lengths
• enums:
  jerry_debugger_flags_t, jerry_debugger_header_type_t, jerry_debugger_configuration_flags_t, jerry_debugger_eval_type_t, jerry_debugger_eval_result_type_t, jerry_debugger_output_subtype_t, ecma_init_flag_t, ecma_status_flag_t, ecma_parse_opts_t, ecma_property_types_t, ecma_list_properties_options_t, ecma_property_flags_t, ecma_collection_flag_t, ecma_direct_string_type_t, ecma_arraybuffer_extra_flag_t, ecma_property_hashmap_delete_status, ecma_builtin_property_type_t, ecma_builtin_number_type_t, ecma_array_object_set_length_flags_t, jerry_init_flag_t, jerry_type_t, jerry_typedarray_type_t, jerry_debugger_wait_for_source_status_t, jerry_generate_snapshot_opts_t, jerry_exec_snapshot_opts_t, jmem_free_unused_memory_severity_t, cbc_code_flags, cbc_opcode_t, cbc_ext_opcode_t, lexer_literal_type_t, lexer_literal_status_flags_t, skip_mode_t, lexer_token_type_t, lexer_obj_ident_opts_t, lexer_literal_object_type_t, lexer_number_type_t, parser_object_literal_item_types_t, parser_general_flags_t, parser_expression_flags_t, scan_modes_t, scan_stack_modes_t, parser_statement_type_t, parser_try_block_type_t, re_token_type_t, number_arithmetic_op, number_bitwise_logic_op, vm_stack_context_type_t, vm_oc_get_types, vm_oc_types, vm_oc_put_types, vm_call_operation

That's quite some, I think. But not universal, for sure. The style is mixed in the code base. But if I had to choose between future proof and not future proof, I'd go for the first option. And definitely would not change back from the first style to the latter one.

[akosthekiss]

Is this function OK here? parser_emit_s used to be at the beginning of the source, just like parser_emit_unary_lvalue_opcode is.

[zherczeg]

I don't think there is such rule. This is a static inline helper, usually defined before use.

[akosthekiss]

Well, I was not referring to rules as such but to coding patterns, especially related to parser_emit_ helper functions. Such functions appear in two files: js-parser-expr.c and js-parser-util.c. In js-parser-util.c, all parser_emit_ functions are grouped to the beginning of the file, just like the single parser_emit_unary_lvalue_opcode helper in js-parser-expr.c (this file). Moreover parser_emit_unary_lvalue_opcode is first used at line 1693 but already defined at line 86 (i.e., not before the use).

[LaszloLango]

@zherczeg please rebase the PR

[zherczeg]

I may reopen it in the future.