Subject: Re: bug#75521: scratch/igc: Delete unused macro DEFVAR_LISP_NOPROX

From: Pip Cet <[email-masked]>
Date: Mon, 13 Jan 2025 19:57:17 +0000

"Eli Zaretskii" <[email-masked]> writes:

>> Date: Mon, 13 Jan 2025 19:18:59 +0000
>> From: Pip Cet <[email-masked]>
>> Cc: [email-masked], [email-masked]
>>
>> "Eli Zaretskii" <[email-masked]> writes:
>>
>> > So you are saying that the problem that we don't assign the new value
>> > (received from vconcat) to the appropriate variable
>> > (Vfont_weight_table etc.)?
>>
>> My first patch assumes that behavior was intentional, and retains it; my
>> second patch assumes it wasn't, and changes things so font_style_table
>> is no longer needed, because we just use Vfont_weight_table directly in
>> its place.
>
> If we remove font_style_table, then all the code in font.c which uses
> that table and accesses Vfont_weight_table etc. by indexing into
> font_style_table, will stop working, no?

See my second patch, it's not a huge change.

> So I think I'd prefer to
> just assign the new value of the table, as returned by vconcat, to the
> corresponding variable (Vfont_weight_table etc.), and leave the rest
> of the code intact.  That sounds like a much smaller change which only
> touches a single place, where the problem is.  Are there any problems
> with this?

Yes, of course there are.  It's a very fragile approach, as evidenced by
the fact that it was broken for so long, and there's absolutely no
benefit to it.  The next person to attempt to change this code won't
understand why we use a non-staticpro'd variable which is kept in sync
with a staticpro'd one (because there is no reason to do so), and
probably break things again.

>> We should not attempt to keep two different memory locations
>> synchronized by trying to catch all modifications.  That never works,
>> and there's no reason for this complication.
>
> I don't think I follow: which modifications are you talking about?  If
> that's Vfont_weight_table etc., then these 3 are documented to be
> unmodifiable directly.

You mean the comment starting with "FIXME"?  I thought it'd be good to
fix that.  (It's also inaccurate, because the problematic workaround it
describes does not, actually, work).

The vectors, of course, can be modified, and that will have very strange
and unexpected results.

>> >> > GC can only free objects that are not reachable from any other object.
>> >>
>> >> No, that's not correct: the value of a forwarded symbol is not marked by
>> >> alloc.c GC.
>> >>
>> >> 	      case SYMBOL_FORWARDED:
>> >> 		/* If the value is forwarded to a buffer or keyboard field,
>> >> 		   these are marked when we see the corresponding object.
>> >> 		   And if it's forwarded to a C variable, either it's not
>> >> 		   a Lisp_Object var, or it's staticpro'd already, or it's
>> >> 		   reachable from font_style_table which is also
>> >> 		   staticpro'd.  */
>> >> 		break;
>> >>
>> >> That comment is incorrect, because font_style_table may contain a
>> >> newly-consed vector, not the one that's still referred to as
>> >> font-weight-table.
>> >
>> > Sorry, you lost me here.
>>
>> As I've tried to explain a number of times, the problem is that AREF
>> (font_style_table, 0) isn't necessarily equal to Vfont_weight_table.
>> The comment claims it is, but that's incorrect.
>
> Ah, so it's a separate issue with that comment.

No, it's not!  The comment is about the very same issue we've been
"discussing" for so long.

We tried avoiding a staticpro, it failed to work, the right thing to do
is to add the staticpro rather than complicating a fragile workaround
further so it will work for a brief period until the next change breaks
it again.

Pip