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.
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