Re: [PATCH] %nil-handling optimization and fixes v1

[Neil Jerram <neil@ossau.uklinux.net>]

So, finally, here we go with these patches.  (again! :-))

In summary, they all look great, and I just have a few minor comments
(below) on the first one.

But I guess we need to decide on your suggestion about

> (I still believe that these should be changed to versions that handle
>  %nil properly, but await approval on that point, so these patches do
>  not make those changes)

because if we agreed this, some of the changes would be needed, or
wouldn't be needed.  So I'll review the discussions on that next.

Regards,
        Neil

> +/*
> + * IMPORTANT NOTE regarding IFLAG numbering!!!
> + *
> + * Several macros depend upon careful IFLAG numbering of SCM_BOOL_F,
> + * SCM_BOOL_T, SCM_ELISP_NIL, SCM_EOL, and the two SCM_XXX_*_DONT_USE
> + * constants.  In particular:
> + *
> + * - SCM_BOOL_F and SCM_BOOL_T must differ in exactly one bit position.
> + *   (used to implement scm_is_bool_and_not_lisp_nil, aka scm_is_bool)
> + *
> + * - SCM_ELISP_NIL and SCM_BOOL_F must differ in exactly one bit position.
> + *   (used to implement scm_is_false_or_lisp_nil and
> + *    scm_is_true_and_not_lisp_nil)
> + *
> + * - SCM_ELISP_NIL and SCM_EOL must differ in exactly one bit position.
> + *   (used to implement scm_is_null_or_lisp_nil)
> + *
> + * - SCM_ELISP_NIL, SCM_BOOL_F, SCM_EOL, SCM_XXX_ANOTHER_LISP_FALSE_DONT_USE
> + *   must all be equal except for two bit positions.
> + *   (used to implement scm_is_lisp_false)
> + *
> + * - SCM_ELISP_NIL, SCM_BOOL_F, SCM_BOOL_T, SCM_XXX_ANOTHER_BOOLEAN_DONT_USE
> + *   must all be equal except for two bit positions.
> + *   (used to implement scm_is_bool_or_lisp_nil)
> + *
> + * These properties allow the aforementioned macros to be implemented
> + * by bitwise ANDing with a mask and then comparing with a constant,
> + * using as a common basis the macro SCM_MATCHES_BITS_IN_COMMON,
> + * defined below.  The properties are checked at compile-time using
> + * `verify' macros near the top of boolean.c and pairs.c.
> + */

Appreciate the detailed comments.

> +/*
> + * These macros are used for compile-time verification that the
> + * constants have the properties needed for the above macro to work
> + * properly.
> + */
> +#define SCM_WITH_LEAST_SIGNIFICANT_1_BIT_CLEARED(x)  ((x) & ((x)-1))
> +#define SCM_HAS_EXACTLY_ONE_BIT_SET(x)					\
> +  ((x) != 0 && SCM_WITH_LEAST_SIGNIFICANT_1_BIT_CLEARED (x) == 0)

I know they're not needed, but I'd still add some more parentheses
here.

> +#define SCM_HAS_EXACTLY_TWO_BITS_SET(x)					\
> +  (SCM_HAS_EXACTLY_ONE_BIT_SET (SCM_WITH_LEAST_SIGNIFICANT_1_BIT_CLEARED (x)))
> +
> +#define SCM_VALUES_DIFFER_IN_EXACTLY_ONE_BIT_POSITION(a,b)		\
> +  (SCM_HAS_EXACTLY_ONE_BIT_SET (SCM_UNPACK(a) ^ SCM_UNPACK(b)))
> +#define SCM_VALUES_DIFFER_IN_EXACTLY_TWO_BIT_POSITIONS(a,b,c,d)		\
> +  (SCM_HAS_EXACTLY_TWO_BITS_SET ((SCM_UNPACK(a) ^ SCM_UNPACK(b)) |	\
> +                                 (SCM_UNPACK(b) ^ SCM_UNPACK(c)) |	\
> +                                 (SCM_UNPACK(c) ^ SCM_UNPACK(d))))
>  \f

I'd like to make it explicit that these macros are not part of the
public libguile API; and we recently agreed on using the
BUILDING_LIBGUILE macro to do this.  So we just need to put #ifdef
BUILDING_LIBGUILE ... #endif around them.

>  /* Evaluator byte codes ('immediate symbols').  These constants are used only
> diff --git a/libguile/print.c b/libguile/print.c
> index 6c44d59..fd65bf9 100644
> --- a/libguile/print.c
> +++ b/libguile/print.c
> @@ -61,18 +61,17 @@
>  static const char *iflagnames[] =
>  {
>    "#f",
> +  "#nil",  /* Elisp nil value. Should print from elisp as symbol `nil'. */
> +  "#<XXX_ANOTHER_LISP_FALSE_DONT_USE__SHOULD_NOT_EXIST!!>",
> +  "()",
>    "#t",
> +  "#<XXX_ANOTHER_BOOLEAN_DONT_USE__SHOULD_NOT_EXIST!!>",

"SHOULD_NOT_EXIST" might make a future developer think that those
entries should removed from the code.  Maybe add a comment to explain
what it really means, or change to "SHOULD_NEVER_BE_SEEN"?