bug#48264: [PATCH v3 03/15] Add and use BUFFER_DEFAULT_VALUE_P

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Cc: 48264@debbugs.gnu.org
> Date: Fri, 07 May 2021 08:49:59 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> >> From: Spencer Baugh <sbaugh@catern.com>
> >> Date: Thu,  6 May 2021 17:33:34 -0400
> >> Cc: Spencer Baugh <sbaugh@catern.com>
> >> 
> >> This makes the code more clear and allows us to more easily change how
> >> this property is determined.
> >
> > Does it?  Can you explain why you think so?  It looks like we are
> > replacing clear code with an equally clear different code.
> 
> Well, "if (idx > 0)" as a conditional requires a fair bit of digging in
> the implementation of DEFVAR_PER_BUFFER variables to understand.  On the
> other hand, "if (BUFFER_DEFAULT_VALUE_P (offset))" is immediately clear:
> We're checking if this variable has a default value.

It's the other way around here: the test "if (idx > 0)" is clear,
whereas "if (BUFFER_DEFAULT_VALUE_P (offset))" makes me go look up the
definition of the macro, because the name is not expressive enough,
and the argument "offset" doesn't help, either.

> By hiding the implementation detail of "idx", we both remove the need to
> know what idx is, and make it easier to later change the implementation
> (as a later commit does).

I don't want to hide the implementation details, I want the code to
speak for itself.  If you can come up with a change that will make the
code really more clear, fine; otherwise I think we should add comments
there to explain the test.