Re: [PATCH v2 07/16] Take offset not idx in PER_BUFFER_VALUE_P

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 21 Nov 2020 21:34:36 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>, Arnold Noronha <arnold@tdrhq.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Dmitry Gutov <dgutov@yandex.ru>
> 
> This also moves the common "idx == -1" check into PER_BUFFER_VALUE_P.
> If idx == -1, it's guaranteed that the corresponding buffer field has
> a per-buffer value.  We do this check everywhere we call
> PER_BUFFER_VALUE_P, so let's put it in the common code.

But the comment says:

   If a slot in this structure is -1, then even though there may
   be a DEFVAR_PER_BUFFER for the slot, there is no default value for it;
   and the corresponding slot in buffer_defaults is not used.

Doesn't this contradict your assumption, even though the existing uses
of the macro don't?

> This is motivated by later commits which will get rid of idx.

Why do something with idx if we will get rid of it?  I don't see any
purpose in such incremental changes: we will install the changes
regarding buffer-local values as a single changeset, so keeping the
code working after each separate patch in the series is not needed,
and just makes the changes unnecessarily larger.

IOW, let's consider the change which gets rid of idx without this
intermediate step.

> +/* Value is true if the variable with offset OFFSET has a local value
> +   in buffer B.  */
> +
> +INLINE bool
> +PER_BUFFER_VALUE_P (struct buffer *b, int offset)
> +{
> +  int idx = PER_BUFFER_IDX (offset);
> +  eassert (idx == -1 || valid_per_buffer_idx (idx));
> +  return idx == -1 || b->local_flags[idx];
> +}
> +
[...]
> @@ -1440,8 +1440,8 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, 
> Lisp_Object where,
>         {
>           int offset = XBUFFER_OBJFWD (innercontents)->offset;
>           int idx = PER_BUFFER_IDX (offset);
> -         if (idx > 0 && bindflag == SET_INTERNAL_SET
> -             && !PER_BUFFER_VALUE_P (buf, idx))
> +         if (bindflag == SET_INTERNAL_SET
> +             && !PER_BUFFER_VALUE_P (buf, offset))

The original code tests idx for a positive value, but your modified
PER_BUFFER_VALUE_P macro will only check that it's non-negative.
That's not the same thing.