[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#48264: [PATCH v3 03/15] Add and use BUFFER_DEFAULT_VALUE_P
From: |
Eli Zaretskii |
Subject: |
bug#48264: [PATCH v3 03/15] Add and use BUFFER_DEFAULT_VALUE_P |
Date: |
Fri, 07 May 2021 15:58:23 +0300 |
> 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.
- bug#48264: [PATCH v4 12/14] Assert that PER_BUFFER_IDX for Lisp variables is not 0, (continued)
- bug#48264: [PATCH v3 04/15] Combine unnecessarily separate loops in buffer.c, Spencer Baugh, 2021/05/06
- bug#48264: [PATCH v3 03/15] Add and use BUFFER_DEFAULT_VALUE_P, Spencer Baugh, 2021/05/06
- bug#48264: [PATCH v3 03/15] Add and use BUFFER_DEFAULT_VALUE_P, Eli Zaretskii, 2021/05/07
- bug#48264: [PATCH v3 03/15] Add and use BUFFER_DEFAULT_VALUE_P, Spencer Baugh, 2021/05/07
- bug#48264: [PATCH v3 03/15] Add and use BUFFER_DEFAULT_VALUE_P,
Eli Zaretskii <=
- bug#48264: [PATCH v3 03/15] Add and use BUFFER_DEFAULT_VALUE_P, Spencer Baugh, 2021/05/07
- bug#48264: [PATCH v3 03/15] Add and use BUFFER_DEFAULT_VALUE_P, Eli Zaretskii, 2021/05/07
- bug#48264: [PATCH v3 03/15] Add and use BUFFER_DEFAULT_VALUE_P, Spencer Baugh, 2021/05/07
- bug#48264: [PATCH v3 03/15] Add and use BUFFER_DEFAULT_VALUE_P, Eli Zaretskii, 2021/05/07
bug#48264: [PATCH v3 05/15] Add and use KILL_PER_BUFFER_VALUE, Spencer Baugh, 2021/05/06
bug#48264: [PATCH v3 08/15] Set non-buffer-local BVARs to Qunbound, Spencer Baugh, 2021/05/06
bug#48264: [PATCH v3 06/15] Rearrange set_internal for buffer forwarded symbols, Spencer Baugh, 2021/05/06