Re: [PATCH 09/10] Access buffer_defaults in BVAR if there's no local binding

[Stefan Monnier]

> If the buffer_local_flags Lisp_Object is 0, though, it means this
> field in buffer_local_flags wasn't initialized,

Actually, no it means it was explicitly set (otherwise it would be Qnil).
0 means that this slot isn't exposed as a lisp variable (and those
fields don't have a "default value").

> which means this field is always buffer-local (and therefore doesn't
> use buffer_defaults).

That's right.

> @@ -825,9 +825,31 @@ set_per_buffer_value (struct buffer *b, int offset, 
> Lisp_Object value)
>    *(Lisp_Object *)(offset + (char *) b) = value;
>  }
>  
> +INLINE bool
> +SHOULD_USE_BUFFER_DEFAULTS(struct buffer *b, ptrdiff_t offset)
                            ^^
                            SPC

Remember to use a space before opening parens (there are other
occurrences in your patches).

> +{
> +  Lisp_Object obj = *((Lisp_Object *) (offset + (char *) 
> &buffer_local_flags));

I think you want to use `PER_BUFFER_IDX` here.

> +  if (obj.i == 0)
> +    return false;

Please don't use `obj.i`, which relies on the internal representation of 
`Lisp_Object`.
Also, this test is not needed: a zero value here (i.e. obj == Qnil)
would be a bug.
We already test this elsewhere, but even if we don't test it here,
`XFIXNUM` could signal an error if `obj` is not a fixnum, so this test
shouldn't be needed even for debugging purposes.

> +  int idx = XFIXNUM(obj);
> +  if (idx < 1)
> +    return false;
> +  return b->local_flags[idx] == 0;

I'd write it as

    return idx > 0 && !PER_BUFFER_VALUE_P (b, idx);

Largely because I dislike writing the constant `true` or `false`,
especially within `if` branches.

> +INLINE Lisp_Object
> +bvar_get(struct buffer *b, ptrdiff_t offset)
> +{
> +  if (SHOULD_USE_BUFFER_DEFAULTS(b, offset)) {
> +    return per_buffer_value(&buffer_defaults, offset);
> +  } else {
> +    return per_buffer_value(b, offset);
> +  }

Remember also that our coding style puts braces on their own lines like:

    if (SHOULD_USE_BUFFER_DEFAULTS (b, offset))
      {
        return per_buffer_value (&buffer_defaults, offset);
      }
    else
      {
        return per_buffer_value (b, offset);
      }

But here you can just drop the braces (and add the spaces and use
`per_buffer_default`):

    if (SHOULD_USE_BUFFER_DEFAULTS (b, offset))
      return per_buffer_default (offset);
    else
      return per_buffer_value (b, offset);

aka

    return SHOULD_USE_BUFFER_DEFAULTS (b, offset))
           ? per_buffer_value (&buffer_defaults, offset)
           : per_buffer_value (b, offset);

> -#define BVAR(buf, field) ((void)0, (buf)->field ## _)
> +#define BVAR(buf, field) bvar_get(buf, PER_BUFFER_VAR_OFFSET (field))

This will unnecessarily slow down access to those fields which have
an `idx < 1` since I can't imagine the compiler will be able to figure
out that `SHOULD_USE_BUFFER_DEFAULTS` will always return false.

So maybe those fields should be accessed via BVAR_DIRECT (which could
include an `eassert` to make sure that `SHOULD_USE_BUFFER_DEFAULTS`
indeed is false).

Your patch series will also presumably slow down BVAR access to the
other fields since they need to test `SHOULD_USE_BUFFER_DEFAULTS` now,
but I think this is the price to pay.  It should be negligible for
accesses to those vars from Elisp code, but of the 600 or so uses of
BVAR in the current code, I wonder if some of them could show
a measurable speed penalty.


        Stefan