From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: "Miles Bader" Newsgroups: gmane.emacs.devel Subject: Re: face-remapping patch Date: Wed, 28 May 2008 12:30:15 +0900 Message-ID: References: NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Trace: ger.gmane.org 1211945431 5531 80.91.229.12 (28 May 2008 03:30:31 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 28 May 2008 03:30:31 +0000 (UTC) Cc: Emacs-Devel To: "Stefan Monnier" Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed May 28 05:31:12 2008 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1K1CNa-0003zA-LX for ged-emacs-devel@m.gmane.org; Wed, 28 May 2008 05:31:10 +0200 Original-Received: from localhost ([127.0.0.1]:38208 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1K1CMp-00022t-Dh for ged-emacs-devel@m.gmane.org; Tue, 27 May 2008 23:30:23 -0400 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1K1CMk-00022H-Ob for emacs-devel@gnu.org; Tue, 27 May 2008 23:30:18 -0400 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1K1CMj-00021e-9D for emacs-devel@gnu.org; Tue, 27 May 2008 23:30:18 -0400 Original-Received: from [199.232.76.173] (port=51910 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1K1CMj-00021V-0C for emacs-devel@gnu.org; Tue, 27 May 2008 23:30:17 -0400 Original-Received: from an-out-0708.google.com ([209.85.132.246]:47886) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1K1CMi-0007XU-Fb for emacs-devel@gnu.org; Tue, 27 May 2008 23:30:16 -0400 Original-Received: by an-out-0708.google.com with SMTP id c38so573429ana.84 for ; Tue, 27 May 2008 20:30:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references:x-google-sender-auth; bh=1pPnnNa3BI+i3ZLmnIO6xa66L7DmPWQc//24wIgmc+k=; b=SUW4Ql1I3Fj/YAJv/vzR3XhjWp3mTaQwNX4R2R57352asigJ47Tt2AefkGWfmxQWwb+m4hvQP+kdl2Lryyul4e1cfiA5XCiYEKrUOlkKKMTn218VNEISJEIN+o9CEVHp33oRRQmSBo8inSyTRSw0omY8/Ny94210p4QZ4jl4GPE= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references:x-google-sender-auth; b=JG+dIvsI3VwiUXbodn2Qb500nN7M6yunwanocHbltG9CpuZYY7jzTyprCIfzXOBqtVsN/WghV5pDqkPwWbK3mLM4PGo93oGIoHyJkWp72rrFYN9IPSDsxNW9vJ0f6yjyLyGWojBWkV+MJpBBDUgXLDlXu6pSm2mDlzW+Jyb6ybo= Original-Received: by 10.100.108.20 with SMTP id g20mr2734490anc.105.1211945415216; Tue, 27 May 2008 20:30:15 -0700 (PDT) Original-Received: by 10.100.232.19 with HTTP; Tue, 27 May 2008 20:30:15 -0700 (PDT) In-Reply-To: Content-Disposition: inline X-Google-Sender-Auth: 51bfbece52ba6aba X-detected-kernel: by monty-python.gnu.org: Linux 2.6 (newer, 2) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:97865 Archived-At: On Wed, May 28, 2008 at 11:54 AM, Stefan Monnier wrote: > Is there any reason why you added redundant tests > like !NILP (Vface_remapping_alist)? I'd expect at least in my case that > face-remapping-alist would almost never be nil so this extra test would > end up redundant. There's only one such test. :-) Anyway, the default value is nil, and while modes and users might start using it, still, I think it would be nil very commonly. > Why have a `signal_p' arg that's not used? > Also, the ChangeLog entry doesn't make much sense: > > (lookup_named_face_1): Renamed from `lookup_named_face'. Add > `signal_p' argument. Pass new last arg to `get_lface_attributes', and > return -1 if it fails. > (lookup_named_face): Redefined in terms of `lookup_named_face_1'. > > The signal_p arg was there before, it's not added. Am I missing something? Can't exactly say what's going on here, but this code is pretty old, so it's possible the trunk code has been changed since the ChangeLog entry was written. Indeed I remember being annoyed at the conflicts that arose when a bunch of that code got re-arranged at some point.... :-) >> + switch (face_id) >> + { >> + case DEFAULT_FACE_ID: name = Qdefault; break; ... >> + default: >> + return face_id; /* Give up. */ > > Does this default case correspond to a bug? if so shouldn't it `abort'? > If not, shouldn't it obey Vface_remapping_alist? Yeah it's a bug; callers to that function are only supposed to pass one of the "basic" face ids. >> + mapping = assq_no_quit (name, Vface_remapping_alist); >> + if (NILP (mapping)) >> + return face_id; /* Give up. */ >> + >> + remapped_face_id = lookup_named_face_1 (f, name, 0); > > This looks odd: we look up Vface_remapping_alist and then ignore the > actual result (other than its being null or not). Is it just an > optimization to avoid calling lookup_named_face_1? If so, a comment is > in order. Insofar as I remember, yeah, just an "optimization". The assumption is that 99% of the time, there's no remapping (and for most of those faces, it's probably true), and I think this function is called a lot. -Miles -- Do not taunt Happy Fun Ball.