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

[Vibhav Pant <vibhavp@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 10695 bytes --]

On Tue, 2022-11-15 at 08:30 +0800, Po Lu wrote:
> 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,

Thanks for the feedback, I tried to address most of it yesterday.
Additionally, I also pushed a few more rules to .clang-format which
should help both clang-format and clangd with formatting code.

Thanks,
Vibhav

-- 
Vibhav Pant
vibhavp@gmail.com
GPG: 7ED1 D48C 513C A024 BE3A  785F E3FB 28CB 6AB5 9598

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]