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

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Cc: 48264@debbugs.gnu.org
> Date: Fri, 07 May 2021 17:43:51 -0400
> 
> > If the sole purpose is to be able to detect coding mistakes, then
> > there are other possibilities to do that, if the compiler cannot help
> > in a way that leaves the sources readable.
> 
> Hopefully.  Although, I'm not sure this approach is fundamentally
> unreadable?  The field names are already mangled with the trailing "_"
> to stop direct access; this is just further mangling them.

Yes, but it's much more than just the appended _.

Consider also the case of some developer instructing a user to provide
values of these fields in a GDB session: currently we need to tell the
user to use just "p foo->bar_".  With this change, we'd need to make
the user type much more, and possibly also make sure Emacs is compiled
with -g3 to have the macros available to the debugger.

> >> 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.
> >
> > Then I guess the _defaulted_ part is a misnomer?
> 
> Possibly; by "defaulted" I intended to mean that the field is one which
> has a default.  But I freely acknowledge it's not a great name.

So how about using _d_ of _def_instead?  It's much shorter and
expresses the purpose no worse than _defaulted_.

> Keep in mind though, this name isn't exposed to the programmer
> anywhere - it might as well be _ABCDEFGHI_, nothing will change
> outside the definition of the BVAR_DEFAULTED_FIELD macro.

See above: I'd prefer to get rid of the macro for this purpose.

> > Failing that, maybe we should simply have a test to detect the
> > mistakes?  That wouldn't prevent bad code from being compiled, but it
> > should reveal it soon enough, since tests are regularly run on hydra.
> 
> A conditionally-compiled runtime check would be very easy to add - I'd
> just change BVAR to something like:
> 
>   (eassert (EQ (buffer_defaults->field ## _)); (buf)->field ## _)
> 
> Which would make sure that it's not used on anything with a default.
> But of course that's substantially more annoying than a compile time
> check...

I'm not sure I understand why this is much more annoying, can you
elaborate?  We have similar assertions, conditioned on
ENABLE_CHECKING, elsewhere in our macros, like XWINDOW etc, so why not
here?