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))))
>
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"?
