Po Lu <luangruo@yahoo.com> writes:
> Thanks. Some minor style comments below:
>
>> +const size_t block_align = BLOCK_ALIGN;
>
> [...]
>
>> +const size_t float_block_floats_length = FLOAT_BLOCK_SIZE;
>> +const size_t float_block_gcmarkbits_length =
>> + 1 + FLOAT_BLOCK_SIZE / BITS_PER_BITS_WORD;
>
> [...]
>
>> +const size_t cons_block_conses_length = CONS_BLOCK_SIZE;
>> +const size_t cons_block_gcmarkbits_length
>> + = 1 + CONS_BLOCK_SIZE / BITS_PER_BITS_WORD;
>
> These should be defined out with HAVE_NATIVE_COMPILATION. In addition,
> the = must come on a new line. Our style is:
>
> foo_long
> = long_bar ();
>
> instead of:
>
> foo_long =
> long_bar ();
>
>>
>> + mark_stack_push_values (ptr->contents,
>> + size
>> + & PSEUDOVECTOR_SIZE_MASK);
>> + struct Lisp_Native_Comp_Unit *comp_u
>> + = XNATIVE_COMP_UNIT (obj);
>
> Here, you used spaces for indentation instead of tabs.
>
>> + if (comp_u->have_static_lisp_data)
>> + {
>> + eassert (NILP (comp_u->lambda_gc_guard_h) &&
>> + NILP (comp_u->lambda_c_name_idx_h) &&
>> + NILP (comp_u->data_vec) &&
>> + NILP (comp_u->data_impure_vec) &&
>> + comp_u->data_imp_relocs == NULL);
>
> In Emacs code, whenever you feel the temptation to write:
>
> foo_condition () &&
> bar_condition ()
>
> write this instead:
>
> foo_condition ()
> && bar_condition ()
>
>> +static gcc_jit_rvalue *
>> +comp_lisp_const_get_lisp_obj_rval (Lisp_Object obj,
>> + comp_lisp_const_t expr);
>> +static comp_lisp_const_t emit_comp_lisp_obj (Lisp_Object obj,
>> + Lisp_Object alloc_class);
>
> This ought to read:
>
> static gcc_jit_rvalue *comp_lisp_const_get_lisp_obj_rval (Lisp_Object,
> comp_lisp_const_t);
> static comp_lisp_const_t emit_comp_lisp_obj (Lisp_Object, Lisp_Object);
>
>> + n =
>> + emit_binary_op (GCC_JIT_BINARY_OP_PLUS,
>> + comp.emacs_uint_type,
>> + emit_binary_op (GCC_JIT_BINARY_OP_LSHIFT,
>> + comp.emacs_uint_type,
>> + comp.lisp_int0,
>> + emit_rvalue_from_emacs_uint (VALBITS)),
>> + n);
>> +
>
> Please, place the = on a new line.
>
>> +static gcc_jit_rvalue *
>> +emit_make_fixnum (gcc_jit_rvalue *obj)
>> +{
>> + emit_comment ("make_fixnum");
>> + return USE_LSB_TAG
>> + ? emit_make_fixnum_LSB_TAG (obj)
>> + : emit_make_fixnum_MSB_TAG (obj);
>> +}
>
> This should read:
>
> return (USE_LSB_TAG
> ? emit_make_fixnum_LSB_TAG (obj)
> ? emit_make_fixnum_MSB_TAG (obj));
>
> instead.
>
>> +typedef struct {
>> + ptrdiff_t size;
>> + gcc_jit_field *header;
>> + gcc_jit_field *contents;
>> + gcc_jit_type *lisp_vector_type;
>> + gcc_jit_type *contents_type;
>> +} jit_vector_type_t;
>
> Please place the opening brace of this struct on a new line.
>
>> + vec.header =
>> + gcc_jit_context_new_field (comp.ctxt,
>> + NULL,
>> + comp.ptrdiff_type,
>> + "header");
>
> Please place the = on a new line, here, and below:
>
>> + vec.contents =
>> + gcc_jit_context_new_field (comp.ctxt,
>> + NULL,
>> + vec.contents_type,
>> + "contents");
>
>> + = STRING_MULTIBYTE (str)
>> + ? gcc_jit_context_new_rvalue_from_int (comp.ctxt,
>> + comp.ptrdiff_type,
>> + SBYTES (str))
>> + // Mark unibyte strings as immovable, so that pin_string does not
>> + // attempt to modify them.
>> + : gcc_jit_context_new_rvalue_from_int (comp.ctxt,
>> + comp.ptrdiff_type, -3);
>
> When you write:
>
> foo = abcdefg
> ? bcdefghij
> : klmnopqrs
>
> everything around the ternary must be placed in parentheses and indented
> as such.
>
>
>> +static inline bool
>> +comp_func_l_p (Lisp_Object func)
>> +{
>> + return !NILP (CALL1I (comp-func-l-p, func));
>> +}
>> +
>> +static inline bool
>> +comp_func_d_p (Lisp_Object func)
>> +{
>> + return !NILP (CALL1I (comp-func-d-p, func));
>> +}
>
> In general, there is no need to write "inline" in C99: simple
> expressions like these will be inlined anyway, and otherwise, the
> compiler will make its own judgement.
>
>> +typedef struct {
>> + ptrdiff_t cons_block_list_idx;
>> + ptrdiff_t cons_block_conses_idx;
>> +} cons_block_entry_t;
>
> Please place the opening brace of this struct on a new line.
>
>> +static Lisp_Object
>> +cons_block_list_get_block_entry (cons_block_entry_t entry)
>> +{
>> + ptrdiff_t list_idx = XFIXNUM (Flength (comp.cons_block_list)) - 1
>> + - entry.cons_block_list_idx;
>> + return Fnth (make_fixnum (list_idx), comp.cons_block_list);
>> +}
>
> Instead of writing:
>
> foo = 077777777777777
> - 07777777777776;
>
> our coding style is:
>
> foo = (077777777777777
> - 07777777777776)
>
> Please adjust this code accordingly.
>
>> + Lisp_Object block;
>> + if (NILP (comp.float_block_list))
>> + block = push_float_block();
>
> There is a missing space here.
>
>> + if (expr.expr_type == COMP_LISP_CONST_SELF_REPR ||
>> + expr.expr_type == COMP_LISP_CONST_VAR)
>> + return expr.expr.lisp_obj;
>
> Please write:
>
> if (expr.expr_type == COMP_LISP_CONST_SELF_REPR
> || expr.expr_type == COMP_LISP_CONST_VAR)
> return expr.expr.lisp_obj;
>
> instead.
>
>
>> + bool valid = EQ (alloc_class, Qd_default) ||
>> + EQ (alloc_class, Qd_impure) ||
>> + EQ (alloc_class, Qd_ephemeral);
>
> What was said about parentheses earlier applies here too.
>
>> + if (FIXNUMP (obj))
>> + expr = (comp_lisp_const_t){ .expr.lisp_obj
>> + = emit_rvalue_from_lisp_obj (obj),
>> + .const_expr_p = true,
>> + .expr_type = COMP_LISP_CONST_SELF_REPR };
>> + else if (BARE_SYMBOL_P (obj) && c_symbol_p (XBARE_SYMBOL (obj)))
>> + expr
>> + = (comp_lisp_const_t){ .expr.lisp_obj
>> + = emit_rvalue_from_lisp_obj (obj),
>> + .const_expr_p = true,
>> + .expr_type = COMP_LISP_CONST_SELF_REPR };
>
> In this compound literal, please place a space between the part that
> looks like a cast and the initializer list.
>
>> + {
>> + Lisp_Object func =
>> + Fgethash (obj,
>> + CALL1I (comp-ctxt-byte-func-to-func-h, Vcomp_ctxt),
>> + Qnil);
>
> Please place the assignment operator on the new line.
>
>> +const char *lisp_type_name[Lisp_Float + 1] = {
>> + "Lisp_Symbol",
>> + "Lisp_Type_Unused0",
>> + "Lisp_Int0",
>> + "Lisp_Int1",
>> + "Lisp_String",
>> + "Lisp_Vectorlike",
>> + "Lisp_Cons",
>> + "Lisp_Float"
>> +};
>
> Please place the opening brace here on a new line.
>
>> + comp.d_default_rvals =
>> + emit_static_data_container (CALL1I (comp-ctxt-d-default, Vcomp_ctxt),
>> + Qd_default);
>> + comp.d_impure_rvals =
>> + emit_static_data_container (CALL1I (comp-ctxt-d-impure, Vcomp_ctxt),
>> + Qd_impure);
>> + comp.d_ephemeral_rvals =
>> + emit_static_data_container (CALL1I (comp-ctxt-d-ephemeral, Vcomp_ctxt),
>> + Qd_ephemeral);
>> +}
>
> Please put the assignment operators on the new line.
>
>> +#if defined(LIBGCCJIT_HAVE_REFLECTION)
>> \
>> + && defined(LIBGCCJIT_HAVE_CTORS) \
>> + && defined(LIBGCCJIT_HAVE_gcc_jit_type_get_aligned)
>> \
>> + && defined(LIBGCCJIT_HAVE_ALIGNMENT) && USE_STACK_LISP_OBJECTS \
>> + && !defined(GC_CHECK_MARKED_OBJECTS)
>
> Please write:
>
> #if defined (FOO)
>
> or
>
> #if defined FOO
>
> instead of:
>
> #if defined(FOO)
>
> In addition, most of your changes consist of lines indented with tabs
> and spaces, which is correct, but it also contains many lines indented
> solely with spaces. Please fix those.
>
> Thanks.
Hi,
in addition to Po Lu good comments, I think this patch will need (other
than a ChangeLog entry) a cover letter with an in deep explanation of
what is trying to achieve.
Also I see this is based on 1b48e8dde5, but I opposed to that change as
brings code complexity for no real advantages, so I think would be
better to have this patch rebased on current master.
Best Regards
Andrea
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4