bug#48264: [PATCH v3 15/15] Add and use BVAR_FIELD macros

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@catern.com>
>> Date: Thu,  6 May 2021 17:33:46 -0400
>> Cc: Spencer Baugh <sbaugh@catern.com>
>> 
>> diff --git a/src/buffer.c b/src/buffer.c
>> index abf112a898..e048e7559f 100644
>> --- a/src/buffer.c
>> +++ b/src/buffer.c
>> @@ -122,17 +122,17 @@ fix_position (Lisp_Object pos)
>>  static void
>>  bset_abbrev_mode (struct buffer *b, Lisp_Object val)
>>  {
>> -  b->abbrev_mode_ = val;
>> +  b->BVAR_DEFAULTED_FIELD(abbrev_mode) = val;
>
> Yuck!  Can we avoid having a macro in the struct field names?  I'd
> prefer that the BVAR_DEFAULTED_FIELD macro accepted the buffer as
> argument instead (if we need a macro at all; see below).

Ah, yes, I probably should have given a little more explanation of this.
I'm not tied to this approach, if we can think of a better way to make
it statically guaranteed that BVAR is only used with fields that have a
default.

>> --- a/src/buffer.h
>> +++ b/src/buffer.h
>> @@ -280,13 +280,18 @@ struct buffer_text
>>      bool_bf redisplay : 1;
>>    };
>>  
>> +#define BVAR_FIELD(field) field ## _
>> +#define BVAR_DEFAULTED_FIELD(field) field ## _defaulted_
>
> I'm not sure these changes are for the better.  Why not use _ and
> _defaulted_ literally?  The macros don't make code easier to write,
> and at least for me make it harder to understand (because I need to
> look up the macro definition).

Well, _ and _defaulted_ should never be written anywhere - any code that
wants to use a field should run it through BVAR_FIELD or
BVAR_DEFAULTED_FIELD.  This is also why these can't be written to take
the buffer as an argument - that wouldn't work for a few use cases.

>> @@ -714,12 +719,12 @@ XBUFFER (Lisp_Object a)
>>  INLINE void
>>  bset_bidi_paragraph_direction (struct buffer *b, Lisp_Object val)
>>  {
>> -  b->bidi_paragraph_direction_ = val;
>> +  b->BVAR_DEFAULTED_FIELD(bidi_paragraph_direction) = val;
>>  }
>>  INLINE void
>>  bset_cache_long_scans (struct buffer *b, Lisp_Object val)
>>  {
>> -  b->cache_long_scans_ = val;
>> +  b->BVAR_DEFAULTED_FIELD(cache_long_scans) = val;
>>  }
>>  INLINE void
>>  bset_case_canon_table (struct buffer *b, Lisp_Object val)
>> @@ -744,12 +749,12 @@ bset_display_count (struct buffer *b, Lisp_Object val)
>>  INLINE void
>>  bset_left_margin_cols (struct buffer *b, Lisp_Object val)
>>  {
>> -  b->left_margin_cols_ = val;
>> +  b->BVAR_DEFAULTED_FIELD(left_margin_cols) = val;
>>  }
>
> Hmm... I'm not sure I understand the effect of these.  Does it mean C
> code can no longer set the buffer-local value of these variables, only
> the default value?

No, this is purely just changing the name of the fields - it has no
impact on functionality, C code can still set the buffer-local
variables.

>> +#define PER_BUFFER_VAR_DEFAULTED_OFFSET(VAR) \
>> +  offsetof (struct buffer, BVAR_DEFAULTED_FIELD(VAR))
>
> Likewise here: I don't see how such macros make the code more
> readable.  I think they make it less readable.

I agree but I couldn't find a better way to ensure that BVAR and
BVAR_OR_DEFAULT are used on the correct fields.