From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: FACE_FROM_ID vs FACE_OPT_FROM_ID Date: Sat, 25 Jun 2016 10:48:00 +0300 Message-ID: <83eg7l1zgv.fsf@gnu.org> References: <83d1n73c5z.fsf@gnu.org> <576C7D75.4070401@cs.ucla.edu> <8360sz2cpc.fsf@gnu.org> <576CFCFE.8050908@cs.ucla.edu> <83ziqa29gd.fsf@gnu.org> <576D16D3.2000801@cs.ucla.edu> <83twgi1z4m.fsf@gnu.org> <576DA780.4080801@cs.ucla.edu> Reply-To: Eli Zaretskii NNTP-Posting-Host: plane.gmane.org X-Trace: ger.gmane.org 1466840941 19537 80.91.229.3 (25 Jun 2016 07:49:01 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 25 Jun 2016 07:49:01 +0000 (UTC) Cc: emacs-devel@gnu.org To: Paul Eggert Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Jun 25 09:49:00 2016 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1bGiKn-0002gs-67 for ged-emacs-devel@m.gmane.org; Sat, 25 Jun 2016 09:48:57 +0200 Original-Received: from localhost ([::1]:48907 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGiKm-0002PL-BI for ged-emacs-devel@m.gmane.org; Sat, 25 Jun 2016 03:48:56 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:37881) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGiKg-0002P5-GP for emacs-devel@gnu.org; Sat, 25 Jun 2016 03:48:51 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bGiKa-0003kY-TK for emacs-devel@gnu.org; Sat, 25 Jun 2016 03:48:49 -0400 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:46721) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGiKa-0003jv-QY; Sat, 25 Jun 2016 03:48:44 -0400 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:4574 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1bGiKY-0006Dz-Vm; Sat, 25 Jun 2016 03:48:43 -0400 In-reply-to: <576DA780.4080801@cs.ucla.edu> (message from Paul Eggert on Fri, 24 Jun 2016 23:34:56 +0200) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:4830:134:3::e X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:204751 Archived-At: > Cc: emacs-devel@gnu.org > From: Paul Eggert > Date: Fri, 24 Jun 2016 23:34:56 +0200 > > On 06/24/2016 03:43 PM, Eli Zaretskii wrote: > > When ENABLE_CHECKING is not defined, which is what happens by default > > in Emacs builds, we attempt to "do the best we can without crashing", > No, 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. The current FACE_FROM_ID doesn't do that, either, when compiled with ENABLE_CHECKING not defined. 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? > It could be that the font and image caches are special cases that have a > lot of bugs, and that we therefore want some sort of belt-and-suspenders > treatment for them that goes beyond ordinary assertions. I don't see > offhand why that would be, though. Are we getting an unusual number of > bugs in that area, and if so what sort of bugs are they? I don't know how to measure "unusual", so I cannot answer that question. I can tell that 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. I believe the known cases are now solved in the development sources, but I won't be surprised to hear about similar problems that still exist in even more rare use cases. As I wrote above, I'm okay with unconditionally aborting if FACE_FROM_ID ever yields NULL, and the code after that is written under the assumption that it cannot happen. However, the current FACE_FROM_ID doesn't ensure that, so we should make some follow-up changes to ensure that. Also, I don't want two macros (and don't see how this could be achieved by 2 macros anyway). I read the related code too frequently to allow confusing macros into it. 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 OK?