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: Fri, 24 Jun 2016 16:43:05 +0300 Message-ID: <83twgi1z4m.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> Reply-To: Eli Zaretskii NNTP-Posting-Host: plane.gmane.org X-Trace: ger.gmane.org 1466777574 20446 80.91.229.3 (24 Jun 2016 14:12:54 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 24 Jun 2016 14:12:54 +0000 (UTC) Cc: emacs-devel@gnu.org To: Paul Eggert Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Jun 24 16:12:48 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 1bGRqh-0000p1-Ta for ged-emacs-devel@m.gmane.org; Fri, 24 Jun 2016 16:12:48 +0200 Original-Received: from localhost ([::1]:43804 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGRqh-000246-1R for ged-emacs-devel@m.gmane.org; Fri, 24 Jun 2016 10:12:47 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:53155) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGRP0-0003Cu-P7 for emacs-devel@gnu.org; Fri, 24 Jun 2016 09:44:16 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bGROw-0004YT-6w for emacs-devel@gnu.org; Fri, 24 Jun 2016 09:44:10 -0400 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:59617) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGROw-0004YM-3M; Fri, 24 Jun 2016 09:44:06 -0400 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:4008 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1bGROs-0004El-3g; Fri, 24 Jun 2016 09:44:04 -0400 In-reply-to: <576D16D3.2000801@cs.ucla.edu> (message from Paul Eggert on Fri, 24 Jun 2016 13:17:39 +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:204726 Archived-At: > Cc: emacs-devel@gnu.org > From: Paul Eggert > Date: Fri, 24 Jun 2016 13:17:39 +0200 > > On 06/24/2016 12:00 PM, Eli Zaretskii wrote: > >> my reading of this: > >> > >> #ifndef ENABLE_CHECKING > >> # define eassert(cond) ((void) (false && (cond))) /* Check COND compiles. */ > >> > >> is that when ENABLE_CHECKING is not defined, eassert does nothing > >> useful. > > Yes, of course. eassert (X) should appear only in places where X must > be true, i.e., where Emacs is seriously broken if X is false. That's the > standard meaning for assertions. And that is what happens here: if the face ID is not in the face cache, something is broken, and we cannot continue using that face ID. In the ENABLE_CHECKING case, we abort immediately, since that's the meaning of ENABLE_CHECKING: allow early detection of invalid data. 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", thus my suggestion to use the default face, if that is in the cache. If even that is not in the cache, we cannot display anything, so I suggest we abort. If you find the code I proposed after eassert confusing, then we can condition it on ENABLE_CHECKING being not defined. Otherwise, I think my suggestion is better than what we have now, because the current code, when compiled without ENABLE_CHECKING, will attempt to use invalid face IDs. > Another way to put it is that in general the behavior of eassert (X) is > undefined if X is false.When (defined ENABLE_CHECKING && > !suppress_checking), this undefined behavior happens to be a diagnostic > and core dump. Otherwise the undefined behavior is whatever the > underlying system does afterwards; when (!defined ENABLE_CHECKING) this > yields better performance overall in the usual and expected case where > Emacs is working properly. My suggestion was an attempt to make the undefined behavior safer, as much as it's possible. That is in line with the general practice in Emacs: to avoid crashing as much as possible, leaving the user in charge, so she could shut down the session in an orderly fashion, thus minimizing the risk of losing precious work. > It is not necessary to put eassert (X) every place where an expression X > must be true. It's helpful only when we reasonably suspect there might > be a bug in Emacs, and where platforms typically will not immediately > fail for other reasons when X is false. So, before a pointer dereference > *P it's not necessary to do eassert (P != NULL) since typical platforms > immediately dump core when dereferencing a null pointer anyway. That accurately describes the FACE_FROM_ID macro we had before these changes, and the way we used it. We could keep that code, but you say the GCC warning about that is useful, so I suggested a way of avoiding the warning that also makes our code safer. After all, if the warning is useful, we should indeed make changes that improve our code, not just shut up the warning. > Conversely, before a subscript operation A[I] it can be helpful to > eassert (0 <= I && I < N), since typical platforms lack reliable > subscript checking. And that's exactly what is missing from current uses of FACE_FROM_ID, when ENABLE_CHECKING is not defined. So, turning back to my suggestion, do you still object to it?