Re: describe-function and advised C functions

[Stefan Monnier <monnier@iro.umontreal.ca>]

> 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