From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: describe-function and advised C functions Date: Wed, 04 Dec 2013 21:38:07 -0500 Message-ID: References: <87txeq8fek.fsf@tsdh.uni-koblenz.de> <87a9ggnaxe.fsf@tsdh.uni-koblenz.de> <8761r4n242.fsf@tsdh.uni-koblenz.de> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1386211104 2798 80.91.229.3 (5 Dec 2013 02:38:24 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 5 Dec 2013 02:38:24 +0000 (UTC) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Dec 05 03:38:30 2013 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 1VoOpi-0004jd-3f for ged-emacs-devel@m.gmane.org; Thu, 05 Dec 2013 03:38:30 +0100 Original-Received: from localhost ([::1]:51471 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VoOph-0008Lq-Oq for ged-emacs-devel@m.gmane.org; Wed, 04 Dec 2013 21:38:29 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:50598) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VoOpW-0008Ki-9N for emacs-devel@gnu.org; Wed, 04 Dec 2013 21:38:25 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VoOpN-0005re-SO for emacs-devel@gnu.org; Wed, 04 Dec 2013 21:38:18 -0500 Original-Received: from ironport2-out.teksavvy.com ([206.248.154.181]:48469) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VoOpN-0005rP-OH for emacs-devel@gnu.org; Wed, 04 Dec 2013 21:38:09 -0500 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: Av8EABK/CFFFxL6g/2dsb2JhbABEuzWDWRdzgh4BAQQBVigLCzQSFBgNiEIGwS2NGkeDKQOIYZwZgV6DFYFJ X-IPAS-Result: Av8EABK/CFFFxL6g/2dsb2JhbABEuzWDWRdzgh4BAQQBVigLCzQSFBgNiEIGwS2NGkeDKQOIYZwZgV6DFYFJ X-IronPort-AV: E=Sophos;i="4.84,565,1355115600"; d="scan'208";a="41174074" Original-Received: from 69-196-190-160.dsl.teksavvy.com (HELO pastel.home) ([69.196.190.160]) by ironport2-out.teksavvy.com with ESMTP/TLS/ADH-AES256-SHA; 04 Dec 2013 21:38:08 -0500 Original-Received: by pastel.home (Postfix, from userid 20848) id 06E2F60F5D; Wed, 4 Dec 2013 21:38:08 -0500 (EST) In-Reply-To: <8761r4n242.fsf@tsdh.uni-koblenz.de> (Tassilo Horn's message of "Wed, 04 Dec 2013 15:05:01 +0100") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3.50 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 206.248.154.181 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 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-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:166103 Archived-At: > Yeah. Here's a patch that removes all ad-* function usages in > help-fns.el which also fixes the original regression. It's a bit hairy, Thanks. It looks pretty good, see nitpicks below. It's pretty messy over all, to some extent because reality is messy, but also because the existing code is already messy, what with def, function, real-function, real-def and no clear description of how they differ from each other. > BTW, why are the advice annotations in the *Help* buffer fontified for > `load', `append', and `concat-seqs', but not for `scheme-mode' and > `quack-mode'. I don't know, but I suspect the reason is not a good one. > - (let* ((advised (and (symbolp function) (featurep 'advice) > - (ad-get-advice-info function))) > + (let* ((advised (and (symbolp function) > + (featurep 'nadvice) > + (advice--p (advice--symbol-function function)))) Looks fine. > ;; If the function is advised, use the symbol that has the > ;; real definition, if that symbol is already set up. > (real-function > (or (and advised > - (let ((origname (cdr (assq 'origname advised)))) > - (and (fboundp origname) origname))) > + (let* ((f function) > + (advised-fn (advice--cdr (advice--symbol-function f)))) > + (while (advice--p advised-fn) > + (setq f advised-fn) > + (setq advised-fn (advice--cdr (if (symbolp f) > + (advice--symbol-function f) > + f)))) > + advised-fn)) Here `f' is unnecessary (you can always replace it either with `function' or with `advised-fn'). Doesn't this break the 80-columns limit? Also, the (symbolp f) test is always nil since (advice--p advised-fn) can't be true at the same time. More important, for an advised macro, your `real-definition' will be a function (either a lambda expression or a byte-code-function-p). > + (aliased (or (symbolp def) > + ;; advised & aliased > + (and (symbolp function) > + (symbolp real-function) > + (not (eq function real-function))))) Please capitalize and punctuate your comments. Also, why not use replace the `and' with (and advised (symbolp real-function)) > + (real-def (cond > + (aliased (let ((f real-function)) > + (while (and (fboundp f) > + (symbolp (symbol-function f))) > + (setq f (symbol-function f))) > + f)) > + ((subrp def) (intern (subr-name def))) > + (t def))) Why do we need `subr-name'? > @@ -567,14 +577,14 @@ > ;; Print what kind of function-like object FUNCTION is. > (princ (cond ((or (stringp def) (vectorp def)) > "a keyboard macro") > + (aliased > + (format "an alias for `%s'" real-def)) > ((subrp def) > (if (eq 'unevalled (cdr (subr-arity def))) > (concat beg "special form") > (concat beg "built-in function"))) > ((byte-code-function-p def) > (concat beg "compiled Lisp function")) > - (aliased > - (format "an alias for `%s'" real-def)) > ((eq (car-safe def) 'lambda) > (concat beg "Lisp function")) > ((eq (car-safe def) 'macro) Hmm... Why was this move necessary? You'll probably want to add a comment explaining it. Stefan