[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: FACE_FROM_ID vs FACE_OPT_FROM_ID
From: |
Paul Eggert |
Subject: |
Re: FACE_FROM_ID vs FACE_OPT_FROM_ID |
Date: |
Sat, 25 Jun 2016 23:34:52 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 |
On 06/25/2016 09:48 AM, Eli Zaretskii wrote:
assertions should be always true; if an assertion fails Emacs should
just abort, and should not try to patch around the bug.
That's not how we've been using eassert in Emacs. Just look around,
and you will see it.
Look around where? Certainly eassert is used that way when
ENABLE_CHECKING is defined. And it would be used that way even if
ENABLE_CHECKING were not defined, if the runtime cost were zero.
Performance concerns should be the only reason we don't have runtime
checking enabled in Emacs in production builds. This is true not only
for eassert, but also for things like subscript checking, uninitialized
variable checking, etc.
The current FACE_FROM_ID doesn't do that, either, when compiled with
ENABLE_CHECKING not defined.
Sure, but that's merely a performance optimization. The intent is that
the predicate must be true in production, just as it must be true when
debugging. Enabling checking should not change the behavior of Emacs,
other than possibly causing core dumps earlier and possibly hurting
performance.
We could, of course, make validate_face I proposed unconditionally
call emacs_abort. Or we could add such code in-line in all places
that don't already have code to deal with a NULL value coming out of
FACE_FROM_ID. Would you agree to such a change?
I'm not quite following the suggestion, but if you're suggesting that
FACE_FROM_ID should eassert that its result is nonnull, that would be
logically OK. In practice, I expect there would be little practical
benefit from this, as callers of FACE_FROM_ID invariably dereference the
result right away, and this reliably dumps core on typical Emacs
platforms anyway. Hence adding the eassert won't catch bugs
significantly earlier than they're already caught.
(Note added in rereading this email before sending it: this idea is
further analyzed below....)
there were a couple of subtle problems in
the display code that under some rare and complicated use cases caused
FACE_FROM_ID to yield NULL where the subsequent code didn't expect
that.
Were these because the ID argument was out of range, or because ID was
in range but the resulting pointer was null? If the former, the current
code already catches that if ENABLE_CHECKING is true; if the latter,
typical platforms already catch that immediately now, as described above.
So, to summarize, here's the proposal.
. Define FACE_FROM_ID as it was before:
#define FACE_FROM_ID(F, ID) \
(UNSIGNED_CMP (ID, <, FRAME_FACE_CACHE (F)->used) \
? FRAME_FACE_CACHE (F)->faces_by_id[ID] \
: NULL)
. In every place that calls FACE_FROM_ID and doesn't already include
code to deal with a NULL value, do this:
face = FACE_FROM_ID (f, face_id);
if (!face)
emacs_abort ();
. Remove FACE_OPT_FROM_ID
If we go this route, I suggest having a function NONNULL_FACE_FROM_ID
that packages up those three lines of code (which will occur in many
places). That is, instead of this:
face = FACE_FROM_ID (f, face_id);
if (!face)
emacs_abort ();
we should write this:
face = NONNULL_FACE_FROM_ID (f, face_id);
to mean the same thing. (Or perhaps you can think of a better name than
NONNULL_FACE_FROM_ID.)
This sort of thing should also suffice to remove the warning. A downside
is that it inserts unnecessary runtime checks in production code, as the
"if (!face) emacs_abort ();" won't detect errors in production usefully
earlier than the existing code does when it dereferences the pointer. In
practice the unnecessary checks probably won't cost that much, so it's
OK if nobody else cares about the performance degradation in production
code. (I tried hard to avoid slowing down Emacs merely to pacify GCC,
and if we go this route we'd be departing slightly from that tradition.)
If you like this idea I can prepare a more-detailed proposal along these
lines.
- FACE_FROM_ID vs FACE_OPT_FROM_ID, Eli Zaretskii, 2016/06/23
- Re: FACE_FROM_ID vs FACE_OPT_FROM_ID, Paul Eggert, 2016/06/23
- Re: FACE_FROM_ID vs FACE_OPT_FROM_ID, Eli Zaretskii, 2016/06/24
- Re: FACE_FROM_ID vs FACE_OPT_FROM_ID, Paul Eggert, 2016/06/24
- Re: FACE_FROM_ID vs FACE_OPT_FROM_ID, Eli Zaretskii, 2016/06/24
- Re: FACE_FROM_ID vs FACE_OPT_FROM_ID, Eli Zaretskii, 2016/06/24
- Re: FACE_FROM_ID vs FACE_OPT_FROM_ID, Paul Eggert, 2016/06/24
- Re: FACE_FROM_ID vs FACE_OPT_FROM_ID, Eli Zaretskii, 2016/06/24
- Re: FACE_FROM_ID vs FACE_OPT_FROM_ID, Paul Eggert, 2016/06/24
- Re: FACE_FROM_ID vs FACE_OPT_FROM_ID, Eli Zaretskii, 2016/06/25
- Re: FACE_FROM_ID vs FACE_OPT_FROM_ID,
Paul Eggert <=