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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 78813 in the body.
You can then email your comments to 78813 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Helmut Eller <eller.helmut <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: feature/igc: [PATCH] Make the array face_attr_sym readonly
Date: Tue, 17 Jun 2025 10:49:10 +0200

[Message part 1 (text/plain, inline)]

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.

[0001-Make-the-array-face_attr_sym-readonly.patch (text/x-diff, attachment)]

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

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

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

There are alternatives, such as using switch statements rather than a
const array, which is what the rest of the code in xfaces.c does.

In any case, we should also add a comment explaining the unusual GC
situation, unless we can resolve it entirely by using a staticpro'd
vector or something.

Pip

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>
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: Tue, 17 Jun 2025 18:36:14 +0300

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

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

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>
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 13:38:57 +0300

> Date: Fri, 20 Jun 2025 10:17:46 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: eller.helmut <at> gmail.com, 78813 <at> debbugs.gnu.org
> 
> "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.

Thanks, but I asked about this change on its own, not as part of some
effort (which AFAIU is on the igc branch, not on master).  IOW,
suppose we release Emacs 31 without the MPS support (something I hope
will not need to happen, but just for the sake of this sub-thread):
what would that change give us, considering the fact that this code
was there since Emacs 21?

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

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

On Fri, Jun 20 2025, Pip Cet wrote:

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

No.  I just copied the existing code and edited it a bit.

Helmut

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

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

On Fri, Jun 20 2025, Eli Zaretskii wrote:

> Thanks, but I asked about this change on its own, not as part of some
> effort (which AFAIU is on the igc branch, not on master).  IOW,
> suppose we release Emacs 31 without the MPS support (something I hope
> will not need to happen, but just for the sake of this sub-thread):
> what would that change give us, considering the fact that this code
> was there since Emacs 21?

It would add some minor protection against accidental modifications of
the face_attr_sym array.

Helmut

Message #26 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 18:31:32 +0000

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

>> Date: Fri, 20 Jun 2025 10:17:46 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: eller.helmut <at> gmail.com, 78813 <at> debbugs.gnu.org
>>
>> "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.
>
> Thanks, but I asked about this change on its own, not as part of some

That's the first paragraph: mainly, it changes code to be less risky in
its no-need-to-GC assumptions, so the very minor changes which would
cause those assumptions to be violated won't result in very tricky bugs.

> effort (which AFAIU is on the igc branch, not on master).  IOW,

Sorry I'm asking for clarification, but my impression was the igc branch
was to change very little in the !HAVE_MPS case, because general GC
cleanups such as this one make more sense on the main branch: fewer
merge conflicts, we'll see the benefits before merging the branch (or
even if for some reason we never do), a better git history, better test
coverage for pre-push testing.

(This isn't about improving code specific to the old GC; I understand you
don't want it touched and I'm working on the assumption that it'll
become unused or go away entirely at some point.)

If we decide not to reduce the number of static objects at this time and
focus on getting things to work, I can live with that and continue
working on the branch, but I can't speak for Helmut, of course.

Diverting all work that's even slightly related to GC to the feature/igc
branch and keeping master in its current state seems like a very risky
proposal to me.

So what shall we do?

> suppose we release Emacs 31 without the MPS support (something I hope
> will not need to happen, but just for the sake of this sub-thread):
> what would that change give us, considering the fact that this code
> was there since Emacs 21?

Perhaps Helmut can add something to what I wrote above.

Pip

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: 78813 <at> debbugs.gnu.org, pipcet <at> protonmail.com
Subject: Re: bug#78813: feature/igc: [PATCH] Make the array face_attr_sym
 readonly
Date: Sat, 21 Jun 2025 09:56:41 +0300

> From: Helmut Eller <eller.helmut <at> gmail.com>
> Cc: Pip Cet <pipcet <at> protonmail.com>,  78813 <at> debbugs.gnu.org
> Date: Fri, 20 Jun 2025 20:29:40 +0200
> 
> On Fri, Jun 20 2025, Eli Zaretskii wrote:
> 
> > Thanks, but I asked about this change on its own, not as part of some
> > effort (which AFAIU is on the igc branch, not on master).  IOW,
> > suppose we release Emacs 31 without the MPS support (something I hope
> > will not need to happen, but just for the sake of this sub-thread):
> > what would that change give us, considering the fact that this code
> > was there since Emacs 21?
> 
> It would add some minor protection against accidental modifications of
> the face_attr_sym array.

How can it be accidentally modified?  It's a static array not exposed
to Lisp.  Its purpose is to avoid code that mentions the attribute
keywords and their LFACE_* indices literally, as in

  if (attr_filter == LFACE_FOUNDRY_INDEX && EQ (keyword, QCfoundry)
      || attr_filter == LFACE_SWIDTH_INDEX && EQ (keyword, QCwidth)
      || attr_filter == LFACE_HEIGHT_INDEX && EQ (keyword, QCheight)
      || ...

etc.

More generally, the usual style in Emacs is to initialize stuff in
init_foo and syms_of_foo functions, which are called during the
various stages and variants of the initialization and startup.
Initialization using C initializers is used for simple values like
scalars.  The LISP_INITIALLY stuff should be used only very sparingly
and if strictly necessary, because it is very confusing at first
sight.  So I'd very much prefer that we kept the current conventions
for initializing complex variables, especially variables with Lisp
values.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>
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: Sat, 21 Jun 2025 10:02:31 +0300

> Date: Fri, 20 Jun 2025 18:31:32 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: eller.helmut <at> gmail.com, 78813 <at> debbugs.gnu.org
> 
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
> 
> >> > 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.
> >
> > Thanks, but I asked about this change on its own, not as part of some
> 
> That's the first paragraph: mainly, it changes code to be less risky in
> its no-need-to-GC assumptions, so the very minor changes which would
> cause those assumptions to be violated won't result in very tricky bugs.

See my other message where I explained why I'd like not to make such
changes, if that is the only advantage.

> > effort (which AFAIU is on the igc branch, not on master).  IOW,
> 
> Sorry I'm asking for clarification, but my impression was the igc branch
> was to change very little in the !HAVE_MPS case, because general GC
> cleanups such as this one make more sense on the main branch: fewer
> merge conflicts, we'll see the benefits before merging the branch (or
> even if for some reason we never do), a better git history, better test
> coverage for pre-push testing.

That's true in general, but if some change in the non-MPS case makes
the overall code cleaner, there should be no reason not to make it on
the branch.

As for merge conflicts, I'm not afraid of that, especially if we make
changes in an area that doesn't see too many changes (initializations
are generally of that nature).

> If we decide not to reduce the number of static objects at this time and
> focus on getting things to work, I can live with that and continue
> working on the branch, but I can't speak for Helmut, of course.
> 
> Diverting all work that's even slightly related to GC to the feature/igc
> branch and keeping master in its current state seems like a very risky
> proposal to me.

Maybe I'm missing something, but what does this change have to do with
GC?  This particular static object is never modified and is not
exposed to Lisp.  So it shouldn't cause us any GC-related efforts.

Message #35 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: Sat, 21 Jun 2025 10:07:43 +0000

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

>> Date: Fri, 20 Jun 2025 18:31:32 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: eller.helmut <at> gmail.com, 78813 <at> debbugs.gnu.org
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> >> > 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.
>> >
>> > Thanks, but I asked about this change on its own, not as part of some
>>
>> That's the first paragraph: mainly, it changes code to be less risky in
>> its no-need-to-GC assumptions, so the very minor changes which would
>> cause those assumptions to be violated won't result in very tricky bugs.
>
> See my other message where I explained why I'd like not to make such
> changes, if that is the only advantage.

>> > effort (which AFAIU is on the igc branch, not on master).  IOW,
>>
>> Sorry I'm asking for clarification, but my impression was the igc branch
>> was to change very little in the !HAVE_MPS case, because general GC
>> cleanups such as this one make more sense on the main branch: fewer
>> merge conflicts, we'll see the benefits before merging the branch (or
>> even if for some reason we never do), a better git history, better test
>> coverage for pre-push testing.
>
> That's true in general, but if some change in the non-MPS case makes
> the overall code cleaner, there should be no reason not to make it on
> the branch.

I can't parse that statement.  If a change affects only the non-MPS
case, which branch should we make it on?  The master branch would make
sense to me, and the feature/igc branch doesn't make sense.

> As for merge conflicts, I'm not afraid of that, especially if we make
> changes in an area that doesn't see too many changes (initializations
> are generally of that nature).

Well, you did just push a change which causes a merge conflict here.

Helmut, as this discussion shows, I see no way to apply changes such as
this one right now; they won't be accepted in master, apparently, and
applying them to feature/igc will produce merge conflicts, as we've just
seen.

>> If we decide not to reduce the number of static objects at this time and
>> focus on getting things to work, I can live with that and continue
>> working on the branch, but I can't speak for Helmut, of course.
>>
>> Diverting all work that's even slightly related to GC to the feature/igc
>> branch and keeping master in its current state seems like a very risky
>> proposal to me.
>
> Maybe I'm missing something, but what does this change have to do with
> GC?  This particular static object is never modified and is not
> exposed to Lisp.  So it shouldn't cause us any GC-related efforts.

This confuses me.  The unusual thing here is precisely that we have a
static object which is not staticpro'd but which we know not to cause GC
problems.  Helmut said so clearly when he posted the patch.  "Never
modified and not exposed to Lisp" is irrelevant to the question of
whether GC needs to know about the object.

This single change isn't critical, of course, but feature/igc already
contains too many changes which should be merged to master before we
ever consider merging the branch as a whole.  If the strategy is to
increase that number further, that makes a sensible merge of feature/igc
much less likely.

Pip

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>
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: Sat, 21 Jun 2025 13:54:38 +0300

> Date: Sat, 21 Jun 2025 10:07:43 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: eller.helmut <at> gmail.com, 78813 <at> debbugs.gnu.org
> 
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
> 
> >> Sorry I'm asking for clarification, but my impression was the igc branch
> >> was to change very little in the !HAVE_MPS case, because general GC
> >> cleanups such as this one make more sense on the main branch: fewer
> >> merge conflicts, we'll see the benefits before merging the branch (or
> >> even if for some reason we never do), a better git history, better test
> >> coverage for pre-push testing.
> >
> > That's true in general, but if some change in the non-MPS case makes
> > the overall code cleaner, there should be no reason not to make it on
> > the branch.
> 
> I can't parse that statement.  If a change affects only the non-MPS
> case, which branch should we make it on?  The master branch would make
> sense to me, and the feature/igc branch doesn't make sense.

That's not the situation I alluded to.  Your question was about more
general situations: you never said that you are talking about changes
that affect _only_ the non-MPS parts of the code.  I replied with the
situation in mind where some change affects _both_ MPS and non-MPS
parts.  Changes that affect only the non-MPS parts should preferably
installed on master, indeed, but even in that case this is not a
dogma, and we could make exceptions if there are good reasons.

> > As for merge conflicts, I'm not afraid of that, especially if we make
> > changes in an area that doesn't see too many changes (initializations
> > are generally of that nature).
> 
> Well, you did just push a change which causes a merge conflict here.

I didn't install any changes on the igc branch, so I'm not sure how
this is relevant.  I did exactly what you suggested: making any such
changes on master.  What am I missing?

> Helmut, as this discussion shows, I see no way to apply changes such as
> this one right now; they won't be accepted in master, apparently, and
> applying them to feature/igc will produce merge conflicts, as we've just
> seen.

What have we just seen, may I ask??

> >> If we decide not to reduce the number of static objects at this time and
> >> focus on getting things to work, I can live with that and continue
> >> working on the branch, but I can't speak for Helmut, of course.
> >>
> >> Diverting all work that's even slightly related to GC to the feature/igc
> >> branch and keeping master in its current state seems like a very risky
> >> proposal to me.
> >
> > Maybe I'm missing something, but what does this change have to do with
> > GC?  This particular static object is never modified and is not
> > exposed to Lisp.  So it shouldn't cause us any GC-related efforts.
> 
> This confuses me.  The unusual thing here is precisely that we have a
> static object which is not staticpro'd but which we know not to cause GC
> problems.  Helmut said so clearly when he posted the patch.  "Never
> modified and not exposed to Lisp" is irrelevant to the question of
> whether GC needs to know about the object.

Well, the initialization aspects are still relevant.  Let's please try
to keep the initialization of Lisp values in the syms_of_FOO and
init_FOO functions, as we always did.  If this requires GC protection,
it's fine by me.

> This single change isn't critical, of course, but feature/igc already
> contains too many changes which should be merged to master before we
> ever consider merging the branch as a whole.  If the strategy is to
> increase that number further, that makes a sensible merge of feature/igc
> much less likely.

Like I said: merge conflicts are a non-issue with modern dVCSes.
Let's not let that particular tail wag the dog.

Message #41 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: Sun, 22 Jun 2025 06:34:11 +0000

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

>> Date: Sat, 21 Jun 2025 10:07:43 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: eller.helmut <at> gmail.com, 78813 <at> debbugs.gnu.org
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> >> Sorry I'm asking for clarification, but my impression was the igc branch
>> >> was to change very little in the !HAVE_MPS case, because general GC
>> >> cleanups such as this one make more sense on the main branch: fewer
>> >> merge conflicts, we'll see the benefits before merging the branch (or
>> >> even if for some reason we never do), a better git history, better test
>> >> coverage for pre-push testing.
>> >
>> > That's true in general, but if some change in the non-MPS case makes
>> > the overall code cleaner, there should be no reason not to make it on
>> > the branch.

This is what you said: a "change in the non-MPS case" (so presumably one
not touching the MPS code) should be made "on the branch".

>> I can't parse that statement.  If a change affects only the non-MPS
>> case, which branch should we make it on?  The master branch would make
>> sense to me, and the feature/igc branch doesn't make sense.
>
> That's not the situation I alluded to.  Your question was about more
> general situations: you never said that you are talking about changes
> that affect _only_ the non-MPS parts of the code.  I replied with the
> situation in mind where some change affects _both_ MPS and non-MPS
> parts.

How is it a "change in the non-MPS case" if it affects both?  I was
merely asking for clarification.

>> This single change isn't critical, of course, but feature/igc already
>> contains too many changes which should be merged to master before we
>> ever consider merging the branch as a whole.  If the strategy is to
>> increase that number further, that makes a sensible merge of feature/igc
>> much less likely.
>
> Like I said: merge conflicts are a non-issue with modern dVCSes.

That's not my experience.

Pip

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>
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: Sun, 22 Jun 2025 10:17:39 +0300

> Date: Sun, 22 Jun 2025 06:34:11 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: eller.helmut <at> gmail.com, 78813 <at> debbugs.gnu.org
> 
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
> 
> >> Date: Sat, 21 Jun 2025 10:07:43 +0000
> >> From: Pip Cet <pipcet <at> protonmail.com>
> >> Cc: eller.helmut <at> gmail.com, 78813 <at> debbugs.gnu.org
> >>
> >> "Eli Zaretskii" <eliz <at> gnu.org> writes:
> >>
> >> >> Sorry I'm asking for clarification, but my impression was the igc branch
> >> >> was to change very little in the !HAVE_MPS case, because general GC
> >> >> cleanups such as this one make more sense on the main branch: fewer
> >> >> merge conflicts, we'll see the benefits before merging the branch (or
> >> >> even if for some reason we never do), a better git history, better test
> >> >> coverage for pre-push testing.
> >> >
> >> > That's true in general, but if some change in the non-MPS case makes
> >> > the overall code cleaner, there should be no reason not to make it on
> >> > the branch.
> 
> This is what you said: a "change in the non-MPS case" (so presumably one
> not touching the MPS code) should be made "on the branch".

No, you presume wrong.  I just mean changes in parts of code that are
not compiled in the MPS build.  But see below.

> >> I can't parse that statement.  If a change affects only the non-MPS
> >> case, which branch should we make it on?  The master branch would make
> >> sense to me, and the feature/igc branch doesn't make sense.
> >
> > That's not the situation I alluded to.  Your question was about more
> > general situations: you never said that you are talking about changes
> > that affect _only_ the non-MPS parts of the code.  I replied with the
> > situation in mind where some change affects _both_ MPS and non-MPS
> > parts.
> 
> How is it a "change in the non-MPS case" if it affects both?

A change could affect both MPS and non-MPS code, couldn't it?  Why is
it so hard to understand?

> >> This single change isn't critical, of course, but feature/igc already
> >> contains too many changes which should be merged to master before we
> >> ever consider merging the branch as a whole.  If the strategy is to
> >> increase that number further, that makes a sensible merge of feature/igc
> >> much less likely.
> >
> > Like I said: merge conflicts are a non-issue with modern dVCSes.
> 
> That's not my experience.

We will have to agree to disagree, then.

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

From: Helmut Eller <eller.helmut <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78813 <at> debbugs.gnu.org, pipcet <at> protonmail.com
Subject: Re: bug#78813: feature/igc: [PATCH] Make the array face_attr_sym
 readonly
Date: Sun, 22 Jun 2025 20:25:44 +0200

On Sat, Jun 21 2025, Eli Zaretskii wrote:

>> From: Helmut Eller <eller.helmut <at> gmail.com>
>> Cc: Pip Cet <pipcet <at> protonmail.com>,  78813 <at> debbugs.gnu.org
>> Date: Fri, 20 Jun 2025 20:29:40 +0200
>> 
>> On Fri, Jun 20 2025, Eli Zaretskii wrote:
>> 
>> > Thanks, but I asked about this change on its own, not as part of some
>> > effort (which AFAIU is on the igc branch, not on master).  IOW,
>> > suppose we release Emacs 31 without the MPS support (something I hope
>> > will not need to happen, but just for the sake of this sub-thread):
>> > what would that change give us, considering the fact that this code
>> > was there since Emacs 21?
>> 
>> It would add some minor protection against accidental modifications of
>> the face_attr_sym array.
>
> How can it be accidentally modified?  It's a static array not exposed
> to Lisp.

I was thinking about accidental modification from C.

[...]
> Initialization using C initializers is used for simple values like
> scalars.  The LISP_INITIALLY stuff should be used only very sparingly
> and if strictly necessary, because it is very confusing at first
> sight.  So I'd very much prefer that we kept the current conventions
> for initializing complex variables, especially variables with Lisp
> values.

This doesn't make much sense to me.  But your project, your rules.

So you can close this bug.

Helmut

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: 78813-done <at> debbugs.gnu.org, pipcet <at> protonmail.com
Subject: Re: bug#78813: feature/igc: [PATCH] Make the array face_attr_sym
 readonly
Date: Mon, 23 Jun 2025 14:15:15 +0300

> From: Helmut Eller <eller.helmut <at> gmail.com>
> Cc: pipcet <at> protonmail.com,  78813 <at> debbugs.gnu.org
> Date: Sun, 22 Jun 2025 20:25:44 +0200
> 
> On Sat, Jun 21 2025, Eli Zaretskii wrote:
> 
> > Initialization using C initializers is used for simple values like
> > scalars.  The LISP_INITIALLY stuff should be used only very sparingly
> > and if strictly necessary, because it is very confusing at first
> > sight.  So I'd very much prefer that we kept the current conventions
> > for initializing complex variables, especially variables with Lisp
> > values.
> 
> This doesn't make much sense to me.  But your project, your rules.
> 
> So you can close this bug.

Done, thanks.

Previous Next

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