Subject: bug#78813: feature/igc: [PATCH] Make the array face_attr_sym readonly

From: Eli Zaretskii <[email-masked]>
Date: Sat, 21 Jun 2025 13:54:38 +0300

> Date: Sat, 21 Jun 2025 10:07:43 +0000
> From: Pip Cet <[email-masked]>
> Cc: [email-masked], [email-masked]
> 
> "Eli Zaretskii" <[email-masked]> 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.