Re: scratch/comp-static-data 5aa3db2f11: comp: Add support for compiling elisp constants into static data.

[Po Lu]

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.