GNU bug report logs -
#75521
scratch/igc: Delete unused macro DEFVAR_LISP_NOPROX
Previous Next
Full log
View this message in rfc822 format
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Date: Tue, 14 Jan 2025 17:02:41 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: stefankangas <at> gmail.com, 75521 <at> debbugs.gnu.org
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> >> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>> >> Date: Mon, 13 Jan 2025 22:07:21 +0200
>> >> From: Eli Zaretskii <eliz <at> gnu.org>
>> >>
>> >>
>> >> Anyway, please don't install anything yet. I want to take a closer
>> >> look at this code and its usage, and I've ran out of time for today.
>> >
>> > WDYT about the patch below? It's supposed to keep the 3 variables in
>> > sync with the corresponding slots in font_style_table, if the slots
>> > are ever modified.
>>
>> If you meant to ask whether the patch will momentarily render this
>> specific bug harmless, I believe it will (but expose other bugs which
>> will still cause crashes).
>
> I was asking about the problem where that code makes the values of
> Vfont_weight_table and its 2 brethren vulnerable to being GC'ed.
> That's the only problem I tried to solve with that patch. That
> Vfont_weight_table etc. are also updated to reflect the changes in
> font_style_table is a nice benefit.
Not really a benefit: updating the variables might expose other
(crashable) bugs more.
> If you agree that this problem will be solved by the patch, then I'd
> like to install it, because it's very simple and localized, so the
> risk to break something is IMO very low.
It introduces what are effectively new crashable bugs by attempting to
fix old ones. I'm neutral: installing this patch won't improve things
significantly, but if you think there might be a slight improvement, you
probably have your reasons.
>> I've already answered the question of what I think about this approach:
>> "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.
>
> I think this wasn't reported because that code is seldom if ever
> executed. I couldn't find a way to enter it. But I see no reason to
> remove it at this point.
No one suggested removing this code.
>> 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."
>
> That can be alleviated a bit by adding commentary which explains this
> subtlety.
The "subtlety" is unnecessary complication, but I repeat myself. We
can't add a comment explaining the reason for it because it's not
necessary.
>> It also changes behavior, of course, and the new behavior doesn't make
>> much sense: with or without --enable-checking, it's trivial to cause
>> further crashes by modifying the Vfont_weight_table variable's value,
>> for example.
>
> Vfont_weight_table etc. are supposed not to be modifiable (it should
> cause Emacs to signal an error). If you can show a recipe for causing
> a crash by modifying them, I will certainly see why we don't behave as
> the doc string says.
??? Of course the vector value can be modified, it's just that the
variable cannot be assigned to. Faset doesn't care, and why would it?
> In any case, this is a separate issue.
Agreed. I'm not asking you not to install your patch (it's code churn,
but then it's possible it'll fix slightly more crashes than it
introduces), and please give me some time to think about whether it's
really worth it to go through another dozen-or-so email cycles just to
end up with another "fix" which makes Emacs even more complicated while
failing to address the actual issues.
Pip
This bug report was last modified 122 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.