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>
>> 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.

Yes, that's fair.

>> >> 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_.

Sure, that would work.

>> 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.

Sure, we could mostly get rid of it, although it's important that the
argument to BVAR_OR_DEFAULT be "case_fold_search" rather than, say,
"case_fold_search_def", even if the field is named the latter.
Otherwise one might accidentally call BVAR with "case_fold_search_def",
which would compile but behave wrong at runtime - and preventing that is
the whole point of the different names.

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

I mean that it's annoying that merely compiling doesn't detect the usage
error, one has to actually run tests.  Since it seems like an easy
mistake to make, that seems like it would be frustrating.  But of course
it's better than nothing and we can make it compile time later on.

If you think such a conditionally-compiled runtime check would be
acceptable for applying these changes, I can go ahead and write that.
I've actually just convinced myself that going with a runtime check at
first and figuring out a compile time check later is a good way to go.