Subject: bug#75318: Lisp_Subr header and layout

From: Eli Zaretskii <[email-masked]>
Date: Wed, 22 Jan 2025 17:27:57 +0200

> Date: Wed, 22 Jan 2025 14:25:22 +0000
> From: Pip Cet <[email-masked]>
> Cc: [email-masked], [email-masked], [email-masked]
> 
> > For example, in the below:
> >
> >>    static union Aligned_Lisp_Subr Swatch_gc_cons_percentage =
> >> -     {{{ PSEUDOVECTOR_FLAG | (PVEC_SUBR << PSEUDOVECTOR_AREA_BITS) },
> >> -       { .a4 = watch_gc_cons_percentage },
> >> -       4, 4, "watch_gc_cons_percentage", {0}, lisp_h_Qnil}};
> >
> > what is lisp_h_Qnil is relatively easy to understand, whereas the
> > replacement, viz.:
> 
> >> +#define SUBR_INITIALIZER(lname, fnname, minargs, maxargs, intspec_value) \
> >> +  {{ .header = SUBR_HEADER,						\
> >> +    .function = { .a ## maxargs = fnname },				\
> >> +    .min_args = minargs,						\
> >> +    .max_args = maxargs,						\
> >> +    .symbol_name = lname,						\
> >> +    .intspec = { .string = intspec_value },				\
> >> +    .command_modes = LISPSYM_INITIALLY (Qnil),			\
> >
> > requires one to look up LISPSYM_INITIALLY, which is
> 
> If you're saying that LISPSYM_INITIALLY is bad, I agree completely.

It isn't bad, it's that replacing lisp_h_Qnil with it makes the code
harder to understand (without going crazy with M-.).

As for LISPSYM_INITIALLY, its name is the main source of confusion:
the INITIALLY part all but prevents any attempt to get a clear idea
what the macro does, without actually looking up all the way to the
basics.  Unless on remembers that by heart, of course (I don't).

> > To make the depth of the proverbial rabbit hole at least to some
> > extent smaller here, could we please refrain from defining
> > SUBR_NATIVECOMP_INITIALIZER, and instead include the necessary stuff
> > directly in SUBR_INITIALIZER, with an #ifdef?  Or do you envision
> > enough other users of SUBR_NATIVECOMP_INITIALIZER to justify yet
> > another macro?
> 
> Do you mean
> 
> #ifdef HAVE_NATIVE_COMP
> #define SUBR_INITIALIZER(lname, fnname, minargs, maxargs, intspec_value) \
>   {{ .header = SUBR_HEADER,						\
>     .function = { .a ## maxargs = fnname },				\
>     .min_args = minargs,						\
>     .max_args = maxargs,						\
>     .symbol_name = lname,						\
>     .intspec = { .string = intspec_value },				\
>     .command_modes = LISPSYM_INITIALLY (Qnil),				\
>     .native_comp_u = LISPSYM_INITIALLY (Qnil),		\
>     .lambda_list = LISPSYM_INITIALLY (Qnil),		\
>     .type = LISPSYM_INITIALLY (Qnil),
>     }}
> #else
> #define SUBR_INITIALIZER(lname, fnname, minargs, maxargs, intspec_value) \
>   {{ .header = SUBR_HEADER,						\
>     .function = { .a ## maxargs = fnname },				\
>     .min_args = minargs,						\
>     .max_args = maxargs,						\
>     .symbol_name = lname,						\
>     .intspec = { .string = intspec_value },				\
>     }}
> #endif
> 
> (and, I assume, the same thing for SUBR_HEADER?)

No.  I meant to use #ifdef inside the macro.  But if that is
impossible in a C macro, I'd even support making this an inline
function.

Anyway, this is a minor comment, so if it's hard to do better, let's
not go overboard.  I'll survive.