unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#12610: unable to use macro in defadvice
@ 2012-10-09 13:02 Le Wang
  2012-10-12  2:05 ` Glenn Morris
  2020-09-13 16:43 ` Lars Ingebrigtsen
  0 siblings, 2 replies; 7+ messages in thread
From: Le Wang @ 2012-10-09 13:02 UTC (permalink / raw)
  To: 12610

Relevant stackoverflow question:
http://stackoverflow.com/questions/12767353/using-macros-in-defadvice

I have this simplified macro.

(defadvice kill-buffer (around show-diff-rephrase-question activate compile)
  "Prompt when a buffer is about to be killed."
  (case (read-char-choice
         "(s/k/q)? "
         (append "sSKkQq" nil))
    ((?s ?S)
     ad-do-it)
    ((?k ?K)
     ad-do-it)
    ((?q ?Q) nil))
  ad-do-it)

If I compile the file and load it, then I get "Invalid function: (115 83)"

It works if I evaluate it.

-- 
Le





^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#12610: unable to use macro in defadvice
  2012-10-09 13:02 bug#12610: unable to use macro in defadvice Le Wang
@ 2012-10-12  2:05 ` Glenn Morris
  2012-10-12  3:46   ` Glenn Morris
  2012-10-12 13:35   ` Stefan Monnier
  2020-09-13 16:43 ` Lars Ingebrigtsen
  1 sibling, 2 replies; 7+ messages in thread
From: Glenn Morris @ 2012-10-12  2:05 UTC (permalink / raw)
  To: Le Wang; +Cc: 12610


Interesting. Advice does not macroexpand the advice definition.
I guess it should?

*** lisp/emacs-lisp/advice.el	2012-09-14 13:44:31 +0000
--- lisp/emacs-lisp/advice.el	2012-10-12 02:02:39 +0000
***************
*** 3658,3664 ****
  	 (advice (ad-make-advice
  		  name (memq 'protect flags)
  		  (not (memq 'disable flags))
! 		  `(advice lambda ,arglist ,@body)))
  	 (preactivation (if (memq 'preactivate flags)
  			    (ad-preactivate-advice
  			     function advice class position))))
--- 3658,3664 ----
  	 (advice (ad-make-advice
  		  name (memq 'protect flags)
  		  (not (memq 'disable flags))
! 		  `(advice lambda ,arglist ,@(macroexpand-all body))))
  	 (preactivation (if (memq 'preactivate flags)
  			    (ad-preactivate-advice
  			     function advice class position))))






^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#12610: unable to use macro in defadvice
  2012-10-12  2:05 ` Glenn Morris
@ 2012-10-12  3:46   ` Glenn Morris
  2012-10-12 13:35   ` Stefan Monnier
  1 sibling, 0 replies; 7+ messages in thread
From: Glenn Morris @ 2012-10-12  3:46 UTC (permalink / raw)
  To: Le Wang; +Cc: 12610


Actually I guess it would be better for the byte-compiler to do the
expansion.





^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#12610: unable to use macro in defadvice
  2012-10-12  2:05 ` Glenn Morris
  2012-10-12  3:46   ` Glenn Morris
@ 2012-10-12 13:35   ` Stefan Monnier
  2012-10-12 17:54     ` Glenn Morris
  1 sibling, 1 reply; 7+ messages in thread
From: Stefan Monnier @ 2012-10-12 13:35 UTC (permalink / raw)
  To: Glenn Morris; +Cc: 12610, Le Wang

> -		  `(advice lambda ,arglist ,@body)))
> +		  `(advice lambda ,arglist ,@(macroexpand-all body))))

I'd rather not do such things manually.  Instead, we should expose the
code as code rather than hide it as data, so that macro-expansion
can take place without calling it explicitly and so the byte-compiler
gets to see the code, optimize it and emit warnings where needed.

I.e. I think the patch should start more along the lines of the one
below (100% guaranteed untested).


        Stefan


=== modified file 'lisp/emacs-lisp/advice.el'
--- lisp/emacs-lisp/advice.el	2012-09-14 13:44:31 +0000
+++ lisp/emacs-lisp/advice.el	2012-10-12 13:25:22 +0000
@@ -3655,13 +3655,13 @@
                      (t (error "defadvice: Invalid or ambiguous flag: %s"
                                flag))))))
 	   args))
-	 (advice (ad-make-advice
-		  name (memq 'protect flags)
-		  (not (memq 'disable flags))
-		  `(advice lambda ,arglist ,@body)))
+	 (advice `(ad-make-advice
+                   ',name ',(memq 'protect flags)
+                   ',(not (memq 'disable flags))
+                   (cons 'advice (lambda ,arglist ,@body))))
 	 (preactivation (if (memq 'preactivate flags)
 			    (ad-preactivate-advice
-			     function advice class position))))
+			     function (eval advice) class position))))
     ;; Now for the things to be done at evaluation time:
     (if (memq 'freeze flags)
 	;; jwz's idea: Freeze the advised definition into a dumpable
@@ -3669,7 +3669,7 @@
 	(ad-make-freeze-definition function advice class position)
         ;; the normal case:
         `(progn
-          (ad-add-advice ',function ',advice ',class ',position)
+          (ad-add-advice ',function ,advice ',class ',position)
           ,@(if preactivation
                 `((ad-set-cache
                    ',function






^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#12610: unable to use macro in defadvice
  2012-10-12 13:35   ` Stefan Monnier
@ 2012-10-12 17:54     ` Glenn Morris
  2012-10-12 18:15       ` Stefan Monnier
  0 siblings, 1 reply; 7+ messages in thread
From: Glenn Morris @ 2012-10-12 17:54 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 12610, Le Wang

Stefan Monnier wrote:

> Instead, we should expose the code as code rather than hide it as
> data, so that macro-expansion can take place without calling it
> explicitly and so the byte-compiler gets to see the code, optimize it
> and emit warnings where needed.

Sounds right.

> I.e. I think the patch should start more along the lines of the one
> below (100% guaranteed untested).

It causes warnings like:

In toplevel form:
uniquify.el:479:13:Warning: assignment to free variable `ad-return-value'
uniquify.el:487:41:Warning: reference to free variable `ad-return-value'

In end of data:
uniquify.el:511:1:Warning: the function `ad-get-arg' is not known to be
    defined.


More importantly, it doesn't work...

emacs -Q -l uniquify

-> byte-code: Symbol's function definition is void: ad-make-advice


Compiling the initial example from this report gives:

(byte-code "<STUFF>" [ad-add-advice kill-buffer ad-make-advice
show-diff-rephrase-question nil t advice #[nil "<STUFF>" [#:--cl-var--
ad-do-it read-char-choice "(s/k/q)? " append "sSKkQq" nil memql (115 83)
(107 75) (113 81)] 5 "Prompt when a buffer is about to be killed."]
around ad-activate] 8)





^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#12610: unable to use macro in defadvice
  2012-10-12 17:54     ` Glenn Morris
@ 2012-10-12 18:15       ` Stefan Monnier
  0 siblings, 0 replies; 7+ messages in thread
From: Stefan Monnier @ 2012-10-12 18:15 UTC (permalink / raw)
  To: Glenn Morris; +Cc: 12610, Le Wang

>> I.e. I think the patch should start more along the lines of the one
>> below (100% guaranteed untested).
[...]
> More importantly, it doesn't work...

That's what I meant by "start".


        Stefan





^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#12610: unable to use macro in defadvice
  2012-10-09 13:02 bug#12610: unable to use macro in defadvice Le Wang
  2012-10-12  2:05 ` Glenn Morris
@ 2020-09-13 16:43 ` Lars Ingebrigtsen
  1 sibling, 0 replies; 7+ messages in thread
From: Lars Ingebrigtsen @ 2020-09-13 16:43 UTC (permalink / raw)
  To: Le Wang; +Cc: 12610

Le Wang <l26wang@gmail.com> writes:

> I have this simplified macro.
>
> (defadvice kill-buffer (around show-diff-rephrase-question activate compile)
>   "Prompt when a buffer is about to be killed."
>   (case (read-char-choice
>          "(s/k/q)? "
>          (append "sSKkQq" nil))
>     ((?s ?S)
>      ad-do-it)
>     ((?k ?K)
>      ad-do-it)
>     ((?q ?Q) nil))
>   ad-do-it)
>
> If I compile the file and load it, then I get "Invalid function: (115 83)"
>
> It works if I evaluate it.

I've modernised the code slightly:

(require 'cl-lib)
	 
(defadvice kill-buffer (around show-diff-rephrase-question activate compile)
  "Prompt when a buffer is about to be killed."
  (cl-case (read-char-choice
            "(s/k/q)? "
            (append "sSKkQq" nil))
    ((?s ?S)
     ad-do-it)
    ((?k ?K)
     ad-do-it)
    ((?q ?Q) nil))
  ad-do-it)

I tried byte-compiling it and loading the .elc file, and it still
doesn't bug out.

So I'm guessing this has been fixed over the years?  I'm closing this
bug report; if you're still seeing this in recent Emacs versions, please
respond to the debbugs address and we'll reopen.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-09-13 16:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-09 13:02 bug#12610: unable to use macro in defadvice Le Wang
2012-10-12  2:05 ` Glenn Morris
2012-10-12  3:46   ` Glenn Morris
2012-10-12 13:35   ` Stefan Monnier
2012-10-12 17:54     ` Glenn Morris
2012-10-12 18:15       ` Stefan Monnier
2020-09-13 16:43 ` Lars Ingebrigtsen

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).