GNU bug report logs - #78813
feature/igc: [PATCH] Make the array face_attr_sym readonly

Previous Next

Full log

Message #14 received at 78813 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78813 <at> debbugs.gnu.org, eller.helmut <at> gmail.com
Subject: Re: bug#78813: feature/igc: [PATCH] Make the array face_attr_sym
 readonly
Date: Fri, 20 Jun 2025 10:17:46 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Date: Tue, 17 Jun 2025 15:28:57 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: 78813 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
>>
>> "Helmut Eller" <eller.helmut <at> gmail.com> writes:
>>
>> > Tags: patch
>> >
>> > This is a proposal to make the variable face_attr_sym const.  Mostly so
>> > that it's more obvious that it doesn't need to be traced by the GC.
>>
>> Makes sense to me, but should be on master.  Is this okay for master?
>
> What are the advantages of making this change on master?

As on feature/igc, the change only makes it harder to break things by
changing the rest of the code; there's no functional change on either
branch, and while Helmut's patch is likely to reduce code size a little,
that's not the reason we should install it.

I don't know whether we actually finally agreed that unprotected static
Lisp_Objects should be phased out, but IIRC that's what Gerd suggested
very strongly.  This change is part of that effort.

I'd prefer LISPSYM_INITIALLY to go away at some point soon, so we can at
least benchmark whether putting lispsym into MPS-managed memory makes
things better or worse.  However, the current code uses
LISPSYM_INITIALLY implicitly, so using it explicitly would make such
changes easier, not harder.

Helmut, I notice both your code and the existing code omit the
LFACE_FAMILY_INDEX case.  Do you happen to know if there's a reason for
that?  If there is, we should add a comment to make sure no one attempts
to reuse this vector in a context where the QCfamily symbol is needed.

An alternative would be to make face_attr_sym a function containing a
switch statement.  Either approach would reduce the different kinds of
unusual static Lisp_Object-containing structures by one: we either use
symbols in code, which is fine, or we do the same thing as the font
drivers (const Lisp_Objects initialized with LISPSYM_INITIALLY).

We could also simply add a comment explaining the unusual situation
(and, possibly, the LFACE_FAMILY_INDEX case).

Pip

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.