unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
       [not found] ` <E1YX3rB-0005WV-PA@vcs.savannah.gnu.org>
@ 2015-03-15  9:12   ` Daniel Colascione
  2015-03-15 15:11     ` Artur Malabarba
                       ` (2 more replies)
  2015-03-15 15:09   ` Artur Malabarba
  1 sibling, 3 replies; 32+ messages in thread
From: Daniel Colascione @ 2015-03-15  9:12 UTC (permalink / raw)
  To: emacs-devel, Tassilo Horn

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

On 03/15/2015 01:25 AM, Tassilo Horn wrote:
> branch: master
> commit 51e7e463e93708a0e40688f91200e9e9869ec662
> Author: Tassilo Horn <tsdh@gnu.org>
> Commit: Tassilo Horn <tsdh@gnu.org>
> 
>     Font-lock elisp macros/special forms dynamically

Is it really a good idea to highlight *all* macros? Many are meant to be
used just like functions and have the same semantics. Instead of
updating a big macro regexp after load, there should be a `declare'
attribute that would let specific macros (and functions) opt into being
fontified specially.

As it is, I'm going to have to disable this functionality locally.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

* Re: master 51e7e46: Font-lock elisp macros/special forms dynamically
       [not found] ` <E1YX3rB-0005WV-PA@vcs.savannah.gnu.org>
  2015-03-15  9:12   ` [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically Daniel Colascione
@ 2015-03-15 15:09   ` Artur Malabarba
  2015-03-16  6:40     ` [Emacs-diffs] " Tassilo Horn
  1 sibling, 1 reply; 32+ messages in thread
From: Artur Malabarba @ 2015-03-15 15:09 UTC (permalink / raw)
  To: emacs-devel, Tassilo Horn; +Cc: emacs-diffs

> +    (unless lisp--el-macro-regexp
> +      (lisp--el-update-macro-regexp))
> +    (add-hook 'after-load-functions #'lisp--el-update-after-load)

Maybe I'm reading this wrong, but is this being automatically added to
`after-load-functions'?
If so, I request we don't do that by default. I haven't tested it yet,
but I'm sure mapping over all atoms and flushing all elisp buffers is
bound to cause noticeable delays whenever a file is loaded.

It could be turned on with a global minor mode.


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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-15  9:12   ` [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically Daniel Colascione
@ 2015-03-15 15:11     ` Artur Malabarba
  2015-03-15 17:20     ` Drew Adams
  2015-03-15 19:15     ` Stefan Monnier
  2 siblings, 0 replies; 32+ messages in thread
From: Artur Malabarba @ 2015-03-15 15:11 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: Tassilo Horn, emacs-devel

> Is it really a good idea to highlight *all* macros? Many are meant to be
> used just like functions and have the same semantics. Instead of
> updating a big macro regexp after load, there should be a `declare'
> attribute that would let specific macros (and functions) opt into being
> fontified specially.

Maybe it should be opt-out. Even if a macro is used similarly to a
function, it behaves differently, and it's good to give the user a
visual notification of that.
(And if the macro behaves exactly like a function it should be a function).



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

* RE: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-15  9:12   ` [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically Daniel Colascione
  2015-03-15 15:11     ` Artur Malabarba
@ 2015-03-15 17:20     ` Drew Adams
  2015-03-15 19:15     ` Stefan Monnier
  2 siblings, 0 replies; 32+ messages in thread
From: Drew Adams @ 2015-03-15 17:20 UTC (permalink / raw)
  To: Daniel Colascione, emacs-devel, Tassilo Horn

> Is it really a good idea to highlight *all* macros? Many are meant to be
> used just like functions and have the same semantics. Instead of
> updating a big macro regexp after load, there should be a `declare'
> attribute that would let specific macros (and functions) opt into being
> fontified specially.
> 
> As it is, I'm going to have to disable this functionality locally.

+1

Just what is this additional highlighting really trying to
accomplish?  What problem is it trying to fix?  Presumably this
corresponds to bug #20096, whose sole motivation was said to be:

"I like the addition of if-let and when-let in subr-x.el. However,
they should also be highlighted as keywords if subr-x is loaded."

To that "I like", the "fix" proposed by Stefan was: "change
elisp-mode's font-lock rules so they check obarray for macros",
and highlight them all.  As Stefan put it: "That should kill
several birds with a single (big) stone."

A big stone, indeed.  But what are the "several birds" that need
to be killed, and why?  No mention.

From two unhighlighted macros that one user wants highlighted,
we've moved to highlighting all macros.  No discussion of the
why-do-this; discussion only of how-to-do-it.

If you must introduce such exhuberent highlighting, perhaps you
would consider throttling it via `font-lock-maximum-decoration'?

(And maybe consider highlighting only those two "I like" macros?)

Yes to Daniel's "let specific...opt into being fontified specially".

http://debbugs.gnu.org/cgi/bugreport.cgi?bug=20096



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-15  9:12   ` [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically Daniel Colascione
  2015-03-15 15:11     ` Artur Malabarba
  2015-03-15 17:20     ` Drew Adams
@ 2015-03-15 19:15     ` Stefan Monnier
  2015-03-16  1:35       ` Artur Malabarba
  2015-03-16  7:41       ` Tassilo Horn
  2 siblings, 2 replies; 32+ messages in thread
From: Stefan Monnier @ 2015-03-15 19:15 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: Tassilo Horn, emacs-devel

> Is it really a good idea to highlight *all* macros? Many are meant to be
> used just like functions and have the same semantics.

IMNSHO most of those are mistakes (should use compiler-macros instead).

> Instead of updating a big macro regexp after load, there should be
> a `declare' attribute that would let specific macros (and functions)
> opt into being fontified specially.

Maybe the other way around would be better: add a declaration that says
"this macro faithfully mimicks the behavior of a function".

> As it is, I'm going to have to disable this functionality locally.

I must admit I haven't tried it out yet, so I'm not sure if I'll like
the result.


        Stefan



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-15 19:15     ` Stefan Monnier
@ 2015-03-16  1:35       ` Artur Malabarba
  2015-03-16  3:07         ` Stefan Monnier
  2015-03-16  7:41       ` Tassilo Horn
  1 sibling, 1 reply; 32+ messages in thread
From: Artur Malabarba @ 2015-03-16  1:35 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Daniel Colascione, emacs-devel, Tassilo Horn

My initial reaction was the same as Drew, I was surprised at seing
this implemented without previous discussion. However...


I'm testing this change now and I like it! Of course there needs to be
a way for macros do declare themselves non-special. But I think being
special by default is a good choice.

I also vote for enabling this by default. Disabling an unwanted
feature is 100 times easier than discovering an unknown feature, for
newbies and veterans alike.

Finally, I'm positively surprised at how fast the update function is.
I would have expected some lag, but haven't found any (even though I'm
looking).

2015-03-15 16:15 GMT-03:00 Stefan Monnier <monnier@iro.umontreal.ca>:
>> Is it really a good idea to highlight *all* macros? Many are meant to be
>> used just like functions and have the same semantics.
>
> IMNSHO most of those are mistakes (should use compiler-macros instead).
>
>> Instead of updating a big macro regexp after load, there should be
>> a `declare' attribute that would let specific macros (and functions)
>> opt into being fontified specially.
>
> Maybe the other way around would be better: add a declaration that says
> "this macro faithfully mimicks the behavior of a function".
>
>> As it is, I'm going to have to disable this functionality locally.
>
> I must admit I haven't tried it out yet, so I'm not sure if I'll like
> the result.
>
>
>         Stefan
>



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16  1:35       ` Artur Malabarba
@ 2015-03-16  3:07         ` Stefan Monnier
  2015-03-16  7:07           ` Tassilo Horn
  0 siblings, 1 reply; 32+ messages in thread
From: Stefan Monnier @ 2015-03-16  3:07 UTC (permalink / raw)
  To: Artur Malabarba; +Cc: Daniel Colascione, emacs-devel, Tassilo Horn

> Finally, I'm positively surprised at how fast the update function is.
> I would have expected some lag, but haven't found any (even though I'm
> looking).

Loading Elisp files should be fairly rare.  But there might be cases
where it can be done repeatedly, but if/when faced with such a situation
we should be able to handle it efficiently e.g. by checking the
load-history (make sure the file did include some definitions before we
bother to scan symbols and rebuild the regexp) since such "run-time
loading" probably won't define new functions.


        Stefan



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-15 15:09   ` Artur Malabarba
@ 2015-03-16  6:40     ` Tassilo Horn
  0 siblings, 0 replies; 32+ messages in thread
From: Tassilo Horn @ 2015-03-16  6:40 UTC (permalink / raw)
  To: Artur Malabarba; +Cc: emacs-diffs, emacs-devel

Artur Malabarba <bruce.connor.am@gmail.com> writes:

>> +    (unless lisp--el-macro-regexp
>> +      (lisp--el-update-macro-regexp))
>> +    (add-hook 'after-load-functions #'lisp--el-update-after-load)
>
> Maybe I'm reading this wrong, but is this being automatically added to
> `after-load-functions'?

Yes.

> If so, I request we don't do that by default. I haven't tested it yet,
> but I'm sure mapping over all atoms and flushing all elisp buffers is
> bound to cause noticeable delays whenever a file is loaded.

It only flushes elisp buffers in case the regexp actually changed.  So
if a file got loaded which doesn't define any new macros, no
re-fontification will happen.

Bye,
Tassilo



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16  3:07         ` Stefan Monnier
@ 2015-03-16  7:07           ` Tassilo Horn
  2015-03-16  7:10             ` Daniel Colascione
  2015-03-16 12:56             ` Stefan Monnier
  0 siblings, 2 replies; 32+ messages in thread
From: Tassilo Horn @ 2015-03-16  7:07 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Daniel Colascione, Artur Malabarba, emacs-devel

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> Finally, I'm positively surprised at how fast the update function is.
>> I would have expected some lag, but haven't found any (even though I'm
>> looking).
>
> Loading Elisp files should be fairly rare.  But there might be cases
> where it can be done repeatedly, but if/when faced with such a situation
> we should be able to handle it efficiently e.g. by checking the
> load-history (make sure the file did include some definitions before we
> bother to scan symbols and rebuild the regexp) since such "run-time
> loading" probably won't define new functions.

Like so?

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index b4f87fd..f8591aa 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -198,9 +198,21 @@ Return non-nil only if the old and new value are different."
 	  (concat "(" (regexp-opt elisp-macros t) "\\_>"))
     (not (string= old-regex lisp--el-macro-regexp))))
 
-(defun lisp--el-update-after-load (_file)
+(defun lisp--el-update-after-load (file)
   "Update `lisp--el-macro-regexp' and adjust font-lock in existing buffers."
-  (when (lisp--el-update-macro-regexp)
+  (when (and
+	 ;; Don't trigger when a file gets reloaded.
+	 (string= file (caar load-history))
+	 ;; Test if the recently loaded file defined any new macros.
+	 (let ((load-entries (cdar load-history)))
+	   (catch 'found
+	     (while load-entries
+	       (when (and (consp (car load-entries))
+			  (eq 'defun (caar load-entries))
+			  (macrop (cdar load-entries)))
+		 (throw 'found t))
+	       (setq load-entries (cdr load-entries)))))
+	 (lisp--el-update-macro-regexp))
     (dolist (buf (buffer-list))
       (when (derived-mode-p 'emacs-lisp-mode)
 	(font-lock-flush)))))
--8<---------------cut here---------------end--------------->8---

Bye,
Tassilo



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16  7:07           ` Tassilo Horn
@ 2015-03-16  7:10             ` Daniel Colascione
  2015-03-16  7:54               ` Tassilo Horn
  2015-03-16 12:58               ` Stefan Monnier
  2015-03-16 12:56             ` Stefan Monnier
  1 sibling, 2 replies; 32+ messages in thread
From: Daniel Colascione @ 2015-03-16  7:10 UTC (permalink / raw)
  To: Stefan Monnier, Artur Malabarba, emacs-devel

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]

On 03/16/2015 12:07 AM, Tassilo Horn wrote:
> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
> 
>>> Finally, I'm positively surprised at how fast the update function is.
>>> I would have expected some lag, but haven't found any (even though I'm
>>> looking).
>>
>> Loading Elisp files should be fairly rare.  But there might be cases
>> where it can be done repeatedly, but if/when faced with such a situation
>> we should be able to handle it efficiently e.g. by checking the
>> load-history (make sure the file did include some definitions before we
>> bother to scan symbols and rebuild the regexp) since such "run-time
>> loading" probably won't define new functions.
> 
> Like so?

Instead of doing it this way, why not make a font-lock matcher that
looks at *every* initial sexp atom, calls intern-soft on it, and applies
a face that depends on properties of the found symbol? This way, we'd
update fontification not only after load, but also after eval-defun, and
it'd be easy to make a `declare'-form that provided for exceptions from
the general rule.

cc-mode uses a system like this for keywords, and I think it's what
Stefan was talking about earlier.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-15 19:15     ` Stefan Monnier
  2015-03-16  1:35       ` Artur Malabarba
@ 2015-03-16  7:41       ` Tassilo Horn
  2015-03-16 12:59         ` Stefan Monnier
  1 sibling, 1 reply; 32+ messages in thread
From: Tassilo Horn @ 2015-03-16  7:41 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Daniel Colascione, emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Instead of updating a big macro regexp after load, there should be a
>> `declare' attribute that would let specific macros (and functions)
>> opt into being fontified specially.
>
> Maybe the other way around would be better: add a declaration that
> says "this macro faithfully mimicks the behavior of a function".

Both opt-in and opt-out sound reasonable to me, and I don't have a
preference.  So just tell me what you prefer.

AFAICS, I'd need to add a new `(no-)font-lock-keyword' entry to
`macro-declarations-alist' and then check (get macro
'(no-)font-lock-keyword) in the code building the regex, right?  Or in
case of opt-in, (declare 'font-lock-keyword) could add the symbol to a
list of macros to font-lock as keywords so that we don't need to check
all symbols in `obarray'.

The name (no-)font-lock-keyword is open to discussion, of course.

Also, I'd like to rename the new lisp--el-*-macro-* symbols to
lisp--el-*-keyword-* since they deal with both macros and special forms,
i.e., they deal with stuff font-locked with keyword face.

Bye,
Tassilo



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16  7:10             ` Daniel Colascione
@ 2015-03-16  7:54               ` Tassilo Horn
  2015-03-16  9:36                 ` Tassilo Horn
  2015-03-16 12:58               ` Stefan Monnier
  1 sibling, 1 reply; 32+ messages in thread
From: Tassilo Horn @ 2015-03-16  7:54 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: Stefan Monnier, Artur Malabarba, emacs-devel

[-- Attachment #1: Type: text/plain, Size: 1432 bytes --]

Daniel Colascione <dancol@dancol.org> writes:

>>> Loading Elisp files should be fairly rare.  But there might be cases
>>> where it can be done repeatedly, but if/when faced with such a
>>> situation we should be able to handle it efficiently e.g. by
>>> checking the load-history (make sure the file did include some
>>> definitions before we bother to scan symbols and rebuild the regexp)
>>> since such "run-time loading" probably won't define new functions.
>> 
>> Like so?
>
> Instead of doing it this way, why not make a font-lock matcher that
> looks at *every* initial sexp atom, calls intern-soft on it, and
> applies a face that depends on properties of the found symbol? This
> way, we'd update fontification not only after load, but also after
> eval-defun, and it'd be easy to make a `declare'-form that provided
> for exceptions from the general rule.

Also sounds good to me, and even simpler implementation-wise.  So I'm
happy to implement it that way if nobody comes up with a reason why
that's not the right way.

And we need to decide if we're opt-in, e.g.,

  (defmacro foo ()
    (declare font-lock-keyword) ...)

makes foo highlighted as a keyword, or if we're opt-out, e.g., all
macros are highlighted by default unless declared as

  (defmacro foo ()
    (declare no-font-lock-keyword) ...)

And I'm open to a better declaration names than the ones above.

Bye,
Tassilo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 bytes --]

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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16  7:54               ` Tassilo Horn
@ 2015-03-16  9:36                 ` Tassilo Horn
  0 siblings, 0 replies; 32+ messages in thread
From: Tassilo Horn @ 2015-03-16  9:36 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: Stefan Monnier, Artur Malabarba, emacs-devel

[-- Attachment #1: Type: text/plain, Size: 1999 bytes --]

Tassilo Horn <tsdh@gnu.org> writes:

>> Instead of doing it this way, why not make a font-lock matcher that
>> looks at *every* initial sexp atom, calls intern-soft on it, and
>> applies a face that depends on properties of the found symbol? This
>> way, we'd update fontification not only after load, but also after
>> eval-defun, and it'd be easy to make a `declare'-form that provided
>> for exceptions from the general rule.
>
> Also sounds good to me, and even simpler implementation-wise.  So I'm
> happy to implement it that way if nobody comes up with a reason why
> that's not the right way.

I've just tried that out in a local branch and it works good.  I went
with the opt-in variant for now (but I can change that) so that you
define macros which should be highlighted like so:

--8<---------------cut here---------------start------------->8---
(defmacro if-let (bindings then &rest else)
  (declare (indent 2)
	   (font-lock-keyword t) ;; <=================== HERE!
	   (debug ((&rest (symbolp form)) form body)))
  (when (and (<= (length bindings) 2)
             (not (listp (car bindings))))
    ;; Adjust the single binding case
    (setq bindings (list bindings)))
  `(let* ,(internal--build-bindings bindings)
     (if ,(car (internal--listify (car (last bindings))))
         ,then
       ,@else)))
--8<---------------cut here---------------end--------------->8---

IMHO, that's better than the regexp variant.  However, when a new macro
with `font-lock-keyword' declaration gets defined (or the value is
changed for some existing macro), we still need to flush existing elisp
buffers.  Else, the effect won't be visible there.

I'm not sure from where I should do that.  On a very fine-granular
basis, the function handling the `font-lock-keyword' declaration could
do that.  But that would mean many flushes when a file with many such
macros gets loaded.  Alternatively, an after-load-function could do
that, but that would miss changes manifested with C-c C-e.

Bye,
Tassilo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 bytes --]

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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16  7:07           ` Tassilo Horn
  2015-03-16  7:10             ` Daniel Colascione
@ 2015-03-16 12:56             ` Stefan Monnier
  1 sibling, 0 replies; 32+ messages in thread
From: Stefan Monnier @ 2015-03-16 12:56 UTC (permalink / raw)
  To: Artur Malabarba; +Cc: Daniel Colascione, emacs-devel

> Like so?

For example, yes.

> +	 ;; Don't trigger when a file gets reloaded.
> +	 (string= file (caar load-history))

Tho I wouldn't do that.  By "repeatedly" I didn't mean literally the
same file.  I was thinking more of cases like calling `load' on all the
~/.emacs.d/elpa/**/*-pkgs.el files (which we don't do any more, BTW).


        Stefan



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16  7:10             ` Daniel Colascione
  2015-03-16  7:54               ` Tassilo Horn
@ 2015-03-16 12:58               ` Stefan Monnier
  2015-03-16 14:47                 ` Tassilo Horn
  1 sibling, 1 reply; 32+ messages in thread
From: Stefan Monnier @ 2015-03-16 12:58 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: Artur Malabarba, emacs-devel

> Instead of doing it this way, why not make a font-lock matcher that
> looks at *every* initial sexp atom, calls intern-soft on it,

That would be a valid alternative implementation, of course.
It's not clear it would be better, OTOH.

So until proved otherwise by a concrete problem, I think we're better
off staying with the current implementation.


        Stefan



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16  7:41       ` Tassilo Horn
@ 2015-03-16 12:59         ` Stefan Monnier
  2015-03-16 14:23           ` Artur Malabarba
  0 siblings, 1 reply; 32+ messages in thread
From: Stefan Monnier @ 2015-03-16 12:59 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: emacs-devel

> Both opt-in and opt-out sound reasonable to me, and I don't have a
> preference.  So just tell me what you prefer.

I much prefer opt-out.


        Stefan



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16 12:59         ` Stefan Monnier
@ 2015-03-16 14:23           ` Artur Malabarba
  0 siblings, 0 replies; 32+ messages in thread
From: Artur Malabarba @ 2015-03-16 14:23 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Daniel Colascione, emacs-devel

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

> > Both opt-in and opt-out sound reasonable to me, and I don't have a
> > preference.  So just tell me what you prefer.
>
> I much prefer opt-out.

+1

[-- Attachment #2: Type: text/html, Size: 225 bytes --]

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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16 12:58               ` Stefan Monnier
@ 2015-03-16 14:47                 ` Tassilo Horn
  2015-03-16 17:31                   ` Stefan Monnier
  0 siblings, 1 reply; 32+ messages in thread
From: Tassilo Horn @ 2015-03-16 14:47 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Daniel Colascione, Artur Malabarba, emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Instead of doing it this way, why not make a font-lock matcher that
>> looks at *every* initial sexp atom, calls intern-soft on it,
>
> That would be a valid alternative implementation, of course.
> It's not clear it would be better, OTOH.
>
> So until proved otherwise by a concrete problem, I think we're better
> off staying with the current implementation.

Here's a patch which implements your preferred opt-out variant using
Daniel's suggestion.

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 747a1d6..78db49e 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,15 @@
+2015-03-16  Tassilo Horn  <tsdh@gnu.org>
+
+	* emacs-lisp/byte-run.el (macro-declarations-alist): New
+	declaration no-font-lock-keyword.
+
+	* emacs-lisp/lisp-mode.el (lisp--el-update-after-load)
+	(lisp--el-update-macro-regexp, lisp--el-macro-regexp): Delete
+	functions and defconst.
+	(lisp--el-match-keyword): Rename from lisp--el-match-macro.
+	(lisp-mode-variables): Remove code for updating/initializing
+	lisp--el-macro-regexp.
+
 2015-03-15  Michael Albinus  <michael.albinus@gmx.de>
 
 	* net/tramp-adb.el:
diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index caa7e3d..ea70b11 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -151,7 +151,13 @@ This is used by `declare'.")
              (list 'progn :autoload-end
                    (list 'put (list 'quote name)
                          ''edebug-form-spec (list 'quote spec)))))
-   defun-declarations-alist)
+   (cons
+    (list 'no-font-lock-keyword
+	  #'(lambda (name _args val)
+	      (list 'progn :autoload-end
+		    (list 'put (list 'quote name)
+			  ''no-font-lock-keyword (list 'quote val)))))
+    defun-declarations-alist))
   "List associating properties of macros to their macro expansion.
 Each element of the list takes the form (PROP FUN) where FUN is a function.
 For each (PROP . VALUES) in a macro's declaration, the FUN corresponding
diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index b4f87fd..8039f7a 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -181,32 +181,14 @@
             nil)))
     res))
 
-(defconst lisp--el-macro-regexp nil
-  "A regular expression matching all loaded elisp macros.
-Can be updated using `lisp--el-update-macro-regexp' after new
-macros were defined.")
-
-(defun lisp--el-update-macro-regexp ()
-  "Update `lisp--el-update-macro-regexp' from `obarray'.
-Return non-nil only if the old and new value are different."
-  (let ((old-regex lisp--el-macro-regexp)
-	(elisp-macros nil))
-    (mapatoms (lambda (a)
-		(when (or (macrop a) (special-form-p a))
-		  (push (symbol-name a) elisp-macros))))
-    (setq lisp--el-macro-regexp
-	  (concat "(" (regexp-opt elisp-macros t) "\\_>"))
-    (not (string= old-regex lisp--el-macro-regexp))))
-
-(defun lisp--el-update-after-load (_file)
-  "Update `lisp--el-macro-regexp' and adjust font-lock in existing buffers."
-  (when (lisp--el-update-macro-regexp)
-    (dolist (buf (buffer-list))
-      (when (derived-mode-p 'emacs-lisp-mode)
-	(font-lock-flush)))))
-
-(defun lisp--el-match-macro (limit)
-  (re-search-forward lisp--el-macro-regexp limit t))
+(defun lisp--el-match-keyword (limit)
+  (catch 'found
+    (while (re-search-forward "(\\(\\(?:\\sw\\|\\s_\\)+\\)\\_>" limit t)
+      (let ((sym (intern-soft (match-string 1))))
+	(when (or (special-form-p sym)
+		  (and (macrop sym)
+		       (not (get sym 'no-font-lock-keyword))))
+	  (throw 'found t))))))
 
 (pcase-let
     ((`(,vdefs ,tdefs
@@ -362,7 +344,7 @@ Return non-nil only if the old and new value are different."
      `( ;; Regexp negated char group.
        ("\\[\\(\\^\\)" 1 font-lock-negation-char-face prepend)
        ;; Control structures.  Common Lisp forms.
-       (lisp--el-match-macro . 1)
+       (lisp--el-match-keyword 1 font-lock-keyword-face)
        ;; Exit/Feature symbols as constants.
        (,(concat "(\\(catch\\|throw\\|featurep\\|provide\\|require\\)\\_>"
                  "[ \t']*\\(\\(?:\\sw\\|\\s_\\)+\\)?")
@@ -543,9 +525,6 @@ font-lock keywords will not be case sensitive."
 	   . lisp-font-lock-syntactic-face-function)))
   (setq-local prettify-symbols-alist lisp--prettify-symbols-alist)
   (when elisp
-    (unless lisp--el-macro-regexp
-      (lisp--el-update-macro-regexp))
-    (add-hook 'after-load-functions #'lisp--el-update-after-load)
     (setq-local electric-pair-text-pairs
                 (cons '(?\` . ?\') electric-pair-text-pairs)))
   (setq-local electric-pair-skip-whitespace 'chomp)
--8<---------------cut here---------------end--------------->8---

IMHO, it's better than the regexp-based variant because there's less and
simpler code, and no refresh of the regexp is needed.  I also can't see
any negative point like slowness (testing with our largest elisp file,
vhdl-mode.el).

What the patch is still missing is

  (1) some code to flush font-lock in existing elisp buffers to make
      changes to no-font-lock-keyword declarations effective

  (2) adding (declare (no-font-lock-keyword t)) to all function-like
      macros we have (e.g., push, pushnew)

Bye,
Tassilo



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16 14:47                 ` Tassilo Horn
@ 2015-03-16 17:31                   ` Stefan Monnier
  2015-03-16 20:26                     ` Tassilo Horn
  0 siblings, 1 reply; 32+ messages in thread
From: Stefan Monnier @ 2015-03-16 17:31 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: Artur Malabarba, emacs-devel

> using Daniel's suggestion.

I like the "big optimized regexp" better than the "dynamic lookup via
intern-soft" (should make for more efficient font-locking, tho I have no
idea if that really proves faster in practice and even less of an idea
if the difference would be measurable).

Also, that loses the "update existing buffers after macro definition",
so it's far from "clearly better" w.r.t end-user behavior (and the
relative simplicity advantage will probably end up vanishing if we try
to fill those kinds of differences).

But it's largely a question of bikeshed color, so if you all prefer the
intern-soft approach, go for it.

>   (2) adding (declare (no-font-lock-keyword t)) to all function-like
>       macros we have (e.g., push, pushnew)

push and pushnew aren't function-like, so they *should* be highlighted
as keywords, I think.


        Stefan



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16 17:31                   ` Stefan Monnier
@ 2015-03-16 20:26                     ` Tassilo Horn
  2015-03-16 20:39                       ` Daniel Colascione
  0 siblings, 1 reply; 32+ messages in thread
From: Tassilo Horn @ 2015-03-16 20:26 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Daniel Colascione, Artur Malabarba, emacs-devel

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> using Daniel's suggestion.
>
> I like the "big optimized regexp" better than the "dynamic lookup via
> intern-soft" (should make for more efficient font-locking, tho I have
> no idea if that really proves faster in practice and even less of an
> idea if the difference would be measurable).

I've tried it on the 18.000 LOC vhdl-mode.el.

--8<---------------cut here---------------start------------->8---
;; That's the intern-soft variant
(defun font-lock-benchmark ()
  (interactive)
  (message "Benchmark: %s"
	   (benchmark-run-compiled 30
	     (goto-char (point-min))
	     (while (lisp--el-match-keyword (point-max))))))

(defconst macro-rx
  (concat "(" (let (macros)
		(mapatoms (lambda (a)
			    (when (or (special-form-p a)
				      (macrop a))
			      (push (symbol-name a) macros))))
		(regexp-opt macros t))
	  "\\_>"))

;; That's the optimized-regexp variant
(defun font-lock-benchmark2 ()
  (interactive)
  (message "Benchmark: %s"
	   (benchmark-run-compiled 30
	     (goto-char (point-min))
	     (while (re-search-forward macro-rx nil t)
	       (match-string 1)))))
--8<---------------cut here---------------end--------------->8---

Indeed, the variant with the optimized regexp is more than twice as fast
as the intern-soft approach.  But both variants are so fast that there's
no observable difference.

> Also, that loses the "update existing buffers after macro definition",
> so it's far from "clearly better" w.r.t end-user behavior (and the
> relative simplicity advantage will probably end up vanishing if we try
> to fill those kinds of differences).

Yes, that's what I've said.  The reason why I haven't implemented it yet
is that I'm not sure where to add the relevant code.  With the
intern-soft approach, we can basically adjust fontification as soon as a
new macro is defined or evaluated using C-c C-e because it's cheap (in
contrast to updating existing buffers only when a file is loaded).  But
of course if a file defines 100 macros, I don't want 100 updates of
existing elisp buffers.

Any suggestions how/where to update existing buffers?

I can imagine that `defmacro' starts or resets some idle timer which
updates existing buffers.  But having a byte-run -> lisp-mode dependency
just seems wrong.

> But it's largely a question of bikeshed color, so if you all prefer the
> intern-soft approach, go for it.

Currently it's mostly Daniel vs. you, so let's wait for some more
opinions.

>>   (2) adding (declare (no-font-lock-keyword t)) to all function-like
>>       macros we have (e.g., push, pushnew)
>
> push and pushnew aren't function-like, so they *should* be highlighted
> as keywords, I think.

I guess they are not function-like because they deal with generalized
variables, right?

Bye,
Tassilo



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16 20:26                     ` Tassilo Horn
@ 2015-03-16 20:39                       ` Daniel Colascione
  2015-03-16 20:56                         ` Stefan Monnier
  0 siblings, 1 reply; 32+ messages in thread
From: Daniel Colascione @ 2015-03-16 20:39 UTC (permalink / raw)
  To: Stefan Monnier, Artur Malabarba, emacs-devel

[-- Attachment #1: Type: text/plain, Size: 3696 bytes --]

On 03/16/2015 01:26 PM, Tassilo Horn wrote:
> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
> 
>>> using Daniel's suggestion.
>>
>> I like the "big optimized regexp" better than the "dynamic lookup via
>> intern-soft" (should make for more efficient font-locking, tho I have
>> no idea if that really proves faster in practice and even less of an
>> idea if the difference would be measurable).
> 
> I've tried it on the 18.000 LOC vhdl-mode.el.
> 
> --8<---------------cut here---------------start------------->8---
> ;; That's the intern-soft variant
> (defun font-lock-benchmark ()
>   (interactive)
>   (message "Benchmark: %s"
> 	   (benchmark-run-compiled 30
> 	     (goto-char (point-min))
> 	     (while (lisp--el-match-keyword (point-max))))))
> 
> (defconst macro-rx
>   (concat "(" (let (macros)
> 		(mapatoms (lambda (a)
> 			    (when (or (special-form-p a)
> 				      (macrop a))
> 			      (push (symbol-name a) macros))))
> 		(regexp-opt macros t))
> 	  "\\_>"))
> 
> ;; That's the optimized-regexp variant
> (defun font-lock-benchmark2 ()
>   (interactive)
>   (message "Benchmark: %s"
> 	   (benchmark-run-compiled 30
> 	     (goto-char (point-min))
> 	     (while (re-search-forward macro-rx nil t)
> 	       (match-string 1)))))
> --8<---------------cut here---------------end--------------->8---
> 
> Indeed, the variant with the optimized regexp is more than twice as fast
> as the intern-soft approach.  But both variants are so fast that there's
> no observable difference.
> 
>> Also, that loses the "update existing buffers after macro definition",
>> so it's far from "clearly better" w.r.t end-user behavior (and the
>> relative simplicity advantage will probably end up vanishing if we try
>> to fill those kinds of differences).
> 
> Yes, that's what I've said.  The reason why I haven't implemented it yet
> is that I'm not sure where to add the relevant code.  With the
> intern-soft approach, we can basically adjust fontification as soon as a
> new macro is defined or evaluated using C-c C-e because it's cheap (in
> contrast to updating existing buffers only when a file is loaded).  But
> of course if a file defines 100 macros, I don't want 100 updates of
> existing elisp buffers.
> 
> Any suggestions how/where to update existing buffers?
> 
> I can imagine that `defmacro' starts or resets some idle timer which
> updates existing buffers.  But having a byte-run -> lisp-mode dependency
> just seems wrong.

Isn't font-lock-flush supposed to be cheap? Running it a thousand times
on byte-run.el only takes 3ms. If that's too expensive, we can
ordinarily run font-lock-flush immediately, but during load, bind a
variable that defers the call until it's unbound. Maybe something like
this around load?

(defmacro with-deferred-emacs-lisp-fontification-updates (&rest body)
  `(let ((inhibit-emacs-lisp-fontification-update t))
    ,@body
    (emacs-lisp-font-lock-flush-all-buffers))

> 
>> But it's largely a question of bikeshed color, so if you all prefer the
>> intern-soft approach, go for it.
> 
> Currently it's mostly Daniel vs. you, so let's wait for some more
> opinions.
> 
>>>   (2) adding (declare (no-font-lock-keyword t)) to all function-like
>>>       macros we have (e.g., push, pushnew)
>>
>> push and pushnew aren't function-like, so they *should* be highlighted
>> as keywords, I think.
> 
> I guess they are not function-like because they deal with generalized
> variables, right?

Right. I'm okay with either opt-in or opt-out behavior. Maybe we can use
edebug specs to detect function-like macros in the absence of other
information?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16 20:39                       ` Daniel Colascione
@ 2015-03-16 20:56                         ` Stefan Monnier
  2015-03-17  9:36                           ` Tassilo Horn
  0 siblings, 1 reply; 32+ messages in thread
From: Stefan Monnier @ 2015-03-16 20:56 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: Artur Malabarba, emacs-devel

> > Any suggestions how/where to update existing buffers?

My suggestion would be to do it from after-load-functions, i.e. using
the exact same code as the code used for the alternative implementation ;-)

> Isn't font-lock-flush supposed to be cheap?

If you use jit-lock-mode, yes.  If not, no.

>>> push and pushnew aren't function-like, so they *should* be highlighted
>>> as keywords, I think.
>> I guess they are not function-like because they deal with generalized
>> variables, right?

Right: (push x y) is not the same as (let ((z y)) (push x z)).


        Stefan



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-16 20:56                         ` Stefan Monnier
@ 2015-03-17  9:36                           ` Tassilo Horn
  2015-03-17 10:17                             ` Artur Malabarba
  2015-03-17 16:34                             ` Stefan Monnier
  0 siblings, 2 replies; 32+ messages in thread
From: Tassilo Horn @ 2015-03-17  9:36 UTC (permalink / raw)
  To: emacs-devel

[-- Attachment #1: Type: text/plain, Size: 1620 bytes --]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> > Any suggestions how/where to update existing buffers?
>
> My suggestion would be to do it from after-load-functions, i.e. using
> the exact same code as the code used for the alternative
> implementation ;-)

Yes, that's what I'm doing now.  In addition, `defmacro' does update as
well (in case of a new macro or a redefinition with different
'no-font-lock-keyword declaration).  So with that, you get instant
fontification updates no matter if you C-x C-e a macro definition or
load a file containing macro definitions.

There's at most one update per C-x C-e on a defmacro form, and exactly
one update per file-load.  And updates are cheaper since no regexp has
to be computed.  I've measured it, and compared to `font-lock-flush'
(with `jit-lock-mode' enabled) collecting all macros and builting a
regexp using `regexp-opt' is about 80 times more expensive.

>> Isn't font-lock-flush supposed to be cheap?
>
> If you use jit-lock-mode, yes.  If not, no.

Using it is the default.  Is there a good reason a user might have
disabled it for elisp buffers?

If no one objects, I'm going to install the patch below anytime soon
when the master branch bootstraps again without

  Eager macro-expansion failure: (void-function cl-every)

Of course, then we still need to handle the function-like macros.
Daniel's suggestion of using the `debug' declaration for that doesn't
work (or I don't get it).  At least, having a debug declaration doesn't
imply being "non-function-like", and neither does having no debug
declaration imply being "function-like".

Bye,
Tassilo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-dynamic-elisp-keyword-font-locking.patch --]
[-- Type: text/x-diff, Size: 6831 bytes --]

From 2724f2899a04291e3b4ce5b6f8ddd186165ae40d Mon Sep 17 00:00:00 2001
From: Tassilo Horn <tsdh@gnu.org>
Date: Mon, 16 Mar 2015 10:25:14 +0100
Subject: [PATCH] Improve dynamic elisp keyword font-locking

* emacs-lisp/byte-run.el (macro-declarations-alist): New
declaration no-font-lock-keyword.
(defmacro): Flush font-lock in existing elisp buffers.

* emacs-lisp/lisp-mode.el (lisp--el-update-after-load)
(lisp--el-update-macro-regexp, lisp--el-macro-regexp): Delete
functions and defconst.
(lisp--el-match-keyword): Rename from lisp--el-match-macro.
(lisp--el-font-lock-flush-elisp-buffers): New function.
(lisp-mode-variables): Remove code for updating
lisp--el-macro-regexp, and add
lisp--el-font-lock-flush-elisp-buffers to after-load-functions.
---
 lisp/ChangeLog               | 15 ++++++++++++++
 lisp/emacs-lisp/byte-run.el  | 28 ++++++++++++++++++++-----
 lisp/emacs-lisp/lisp-mode.el | 49 ++++++++++++++++++--------------------------
 3 files changed, 58 insertions(+), 34 deletions(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index cbd1bce..88c3c01 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,18 @@
+2015-03-17  Tassilo Horn  <tsdh@gnu.org>
+
+	* emacs-lisp/byte-run.el (macro-declarations-alist): New
+	declaration no-font-lock-keyword.
+	(defmacro): Flush font-lock in existing elisp buffers.
+
+	* emacs-lisp/lisp-mode.el (lisp--el-update-after-load)
+	(lisp--el-update-macro-regexp, lisp--el-macro-regexp): Delete
+	functions and defconst.
+	(lisp--el-match-keyword): Rename from lisp--el-match-macro.
+	(lisp--el-font-lock-flush-elisp-buffers): New function.
+	(lisp-mode-variables): Remove code for updating
+	lisp--el-macro-regexp, and add
+	lisp--el-font-lock-flush-elisp-buffers to after-load-functions.
+
 2015-03-16  Alan Mackenzie  <acm@muc.de>
 
 	Edebug: Allow "S" to work during trace mode.  Fixes debbugs #20074.
diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index caa7e3d..01c1af7 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -147,11 +147,16 @@ This is used by `declare'.")
 (defvar macro-declarations-alist
   (cons
    (list 'debug
-         #'(lambda (name _args spec)
-             (list 'progn :autoload-end
-                   (list 'put (list 'quote name)
-                         ''edebug-form-spec (list 'quote spec)))))
-   defun-declarations-alist)
+	 #'(lambda (name _args spec)
+	     (list 'progn :autoload-end
+		   (list 'put (list 'quote name)
+			 ''edebug-form-spec (list 'quote spec)))))
+   (cons
+    (list 'no-font-lock-keyword
+	  #'(lambda (name _args val)
+	      (list 'function-put (list 'quote name)
+		    ''no-font-lock-keyword (list 'quote val))))
+    defun-declarations-alist))
   "List associating properties of macros to their macro expansion.
 Each element of the list takes the form (PROP FUN) where FUN is a function.
 For each (PROP . VALUES) in a macro's declaration, the FUN corresponding
@@ -201,6 +206,19 @@ The return value is undefined.
 			  (message "Warning: Unknown macro property %S in %S"
 				   (car x) name))))
 		  decls)))
+	   ;; Refresh font-lock if this is a new macro, or it is an
+	   ;; existing macro whose 'no-font-lock-keyword declaration
+	   ;; has changed.
+	   (if (and
+		;; During bootstrap, subr.el isn't loaded, but then we
+		;; don't need to refresh anyway.
+		(fboundp 'macrop)
+		(fboundp 'lisp--el-font-lock-flush-elisp-buffers)
+		(macrop name)
+		(member `(function-put ',name 'no-font-lock-keyword
+				       ',(get name 'no-font-lock-keyword))
+			declarations))
+	       (lisp--el-font-lock-flush-elisp-buffers))
 	   (if declarations
 	       (cons 'prog1 (cons def declarations))
 	     def))))))
diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index b4f87fd..6b30773 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -181,32 +181,25 @@
             nil)))
     res))
 
-(defconst lisp--el-macro-regexp nil
-  "A regular expression matching all loaded elisp macros.
-Can be updated using `lisp--el-update-macro-regexp' after new
-macros were defined.")
-
-(defun lisp--el-update-macro-regexp ()
-  "Update `lisp--el-update-macro-regexp' from `obarray'.
-Return non-nil only if the old and new value are different."
-  (let ((old-regex lisp--el-macro-regexp)
-	(elisp-macros nil))
-    (mapatoms (lambda (a)
-		(when (or (macrop a) (special-form-p a))
-		  (push (symbol-name a) elisp-macros))))
-    (setq lisp--el-macro-regexp
-	  (concat "(" (regexp-opt elisp-macros t) "\\_>"))
-    (not (string= old-regex lisp--el-macro-regexp))))
-
-(defun lisp--el-update-after-load (_file)
-  "Update `lisp--el-macro-regexp' and adjust font-lock in existing buffers."
-  (when (lisp--el-update-macro-regexp)
+(defun lisp--el-match-keyword (limit)
+  (catch 'found
+    (while (re-search-forward "(\\(\\(?:\\sw\\|\\s_\\)+\\)\\_>" limit t)
+      (let ((sym (intern-soft (match-string 1))))
+	(when (or (special-form-p sym)
+		  (and (macrop sym)
+		       (not (get sym 'no-font-lock-keyword))))
+	  (throw 'found t))))))
+
+(defun lisp--el-font-lock-flush-elisp-buffers (&optional file)
+  ;; Don't flush during load unless called from after-load-functions.
+  ;; In that case, FILE is non-nil.  It's somehow strange that
+  ;; load-in-progress is t when an after-load-function is called since
+  ;; that should run *after* the load...
+  (when (or (not load-in-progress) file)
     (dolist (buf (buffer-list))
-      (when (derived-mode-p 'emacs-lisp-mode)
-	(font-lock-flush)))))
-
-(defun lisp--el-match-macro (limit)
-  (re-search-forward lisp--el-macro-regexp limit t))
+      (with-current-buffer buf
+	(when (derived-mode-p 'emacs-lisp-mode)
+	  (font-lock-flush))))))
 
 (pcase-let
     ((`(,vdefs ,tdefs
@@ -362,7 +355,7 @@ Return non-nil only if the old and new value are different."
      `( ;; Regexp negated char group.
        ("\\[\\(\\^\\)" 1 font-lock-negation-char-face prepend)
        ;; Control structures.  Common Lisp forms.
-       (lisp--el-match-macro . 1)
+       (lisp--el-match-keyword . 1)
        ;; Exit/Feature symbols as constants.
        (,(concat "(\\(catch\\|throw\\|featurep\\|provide\\|require\\)\\_>"
                  "[ \t']*\\(\\(?:\\sw\\|\\s_\\)+\\)?")
@@ -543,9 +536,7 @@ font-lock keywords will not be case sensitive."
 	   . lisp-font-lock-syntactic-face-function)))
   (setq-local prettify-symbols-alist lisp--prettify-symbols-alist)
   (when elisp
-    (unless lisp--el-macro-regexp
-      (lisp--el-update-macro-regexp))
-    (add-hook 'after-load-functions #'lisp--el-update-after-load)
+    (add-hook 'after-load-functions #'lisp--el-font-lock-flush-elisp-buffers)
     (setq-local electric-pair-text-pairs
                 (cons '(?\` . ?\') electric-pair-text-pairs)))
   (setq-local electric-pair-skip-whitespace 'chomp)
-- 
2.3.3


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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-17  9:36                           ` Tassilo Horn
@ 2015-03-17 10:17                             ` Artur Malabarba
  2015-03-17 16:34                             ` Stefan Monnier
  1 sibling, 0 replies; 32+ messages in thread
From: Artur Malabarba @ 2015-03-17 10:17 UTC (permalink / raw)
  To: emacs-devel, Daniel Colascione

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

> Of course, then we still need to handle the function-like macros.
> Daniel's suggestion of using the `debug' declaration for that doesn't
> work (or I don't get it).  At least, having a debug declaration doesn't
> imply being "non-function-like", and neither does having no debug
> declaration imply being "function-like".

Yes, I had thought about suggesting that but it wouldn't work. There are
plenty of very macro-y macros whose arguments are function-like for debug
purpose. See, for instance, with-temp-buffer.

[-- Attachment #2: Type: text/html, Size: 641 bytes --]

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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-17  9:36                           ` Tassilo Horn
  2015-03-17 10:17                             ` Artur Malabarba
@ 2015-03-17 16:34                             ` Stefan Monnier
  2015-03-17 16:47                               ` Nicolas Richard
  2015-03-18  7:17                               ` Tassilo Horn
  1 sibling, 2 replies; 32+ messages in thread
From: Stefan Monnier @ 2015-03-17 16:34 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: Artur Malabarba, emacs-devel

>>> Isn't font-lock-flush supposed to be cheap?
>> If you use jit-lock-mode, yes.  If not, no.
> Using it is the default.  Is there a good reason a user might have
> disabled it for elisp buffers?

There ideally shouldn't be a good reason, no.  It can be very useful
while debugging font-lock-keywords, and things like that, but for
"normal" use, jit-lock-mode should always be used in Elisp buffers.

> If no one objects, I'm going to install the patch below anytime soon
> when the master branch bootstraps again without
>   Eager macro-expansion failure: (void-function cl-every)

Isn't this fixed already (at least, the cl-every I introduced has
disappeared as a side-effect of a subsequent change)?


        Stefan



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-17 16:34                             ` Stefan Monnier
@ 2015-03-17 16:47                               ` Nicolas Richard
  2015-03-18  7:17                               ` Tassilo Horn
  1 sibling, 0 replies; 32+ messages in thread
From: Nicolas Richard @ 2015-03-17 16:47 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Daniel Colascione, Artur Malabarba, emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> Isn't this fixed already (at least, the cl-every I introduced has
> disappeared as a side-effect of a subsequent change)?

I still see it :

$ git --no-pager grep cl-every savannah/master:lisp/emacs-lisp/cl-macs.el 
savannah/master:lisp/emacs-lisp/cl-macs.el:		 (cl-every (lambda (x) (null (cadr x))) (cdr cl--bind-defs)))
$ git rev-parse savannah/master
508049aae95c42a3e6fe989ff403bf7cb6f88433


-- 
Nico



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-17 16:34                             ` Stefan Monnier
  2015-03-17 16:47                               ` Nicolas Richard
@ 2015-03-18  7:17                               ` Tassilo Horn
  2015-03-18  9:10                                 ` Artur Malabarba
  2015-03-18 13:13                                 ` Stefan Monnier
  1 sibling, 2 replies; 32+ messages in thread
From: Tassilo Horn @ 2015-03-18  7:17 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Daniel Colascione, Artur Malabarba, emacs-devel

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> Isn't font-lock-flush supposed to be cheap?
>>> If you use jit-lock-mode, yes.  If not, no.
>> Using it is the default.  Is there a good reason a user might have
>> disabled it for elisp buffers?
>
> There ideally shouldn't be a good reason, no.  It can be very useful
> while debugging font-lock-keywords, and things like that, but for
> "normal" use, jit-lock-mode should always be used in Elisp buffers.

Ok.  I just tried it anyway, and with jit-lock disabled,
`font-lock-flush' does nothing, e.g., the buffer portion you have
already scrolled through is still fontified as it has been before but
the yet "unknown" portions are and stay in the default face when
scrolling to them.  So it seems like in non-jit-locked buffers we'd
actually need to call f-l-flush followed by f-l-ensure.

But OTOH, when someone turns off jit-lock to debug fontification, she
might not like refreshed from the outside so that behavior could be seen
as justified there...

>> If no one objects, I'm going to install the patch below anytime soon
>> when the master branch bootstraps again without
>>   Eager macro-expansion failure: (void-function cl-every)
>
> Isn't this fixed already (at least, the cl-every I introduced has
> disappeared as a side-effect of a subsequent change)?

Yes, yes, its fixed.  I've committed and pushed my last patch (with some
fixes since the posted version) as 9fdc166.

If no one comes up with a (semi-)automated way of identifying
function-like macros, I'll go check the ~1300 macros in emacs manually.
For that, do we have clear criteria when we consider a macro
function-like?  Here's my list so far:

  - function-like:
    + has no arguments
    + just there to save a funcall (ad-get-advice-info-macro,
      shadow-cluster-name)
      Shouldn't those be defsubsts instead of macros?

  - not function-like:
    + is a control structure (when, when-let, cl-loop,...)
    + modifies places (push, pushnew, setf, ...)
    + defines something (def*, define-*)
    + sets up some environment (with-*, save-*)
    + error handling (ignore-errors, condition-case)
    + anaphora

Bye,
Tassilo



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-18  7:17                               ` Tassilo Horn
@ 2015-03-18  9:10                                 ` Artur Malabarba
  2015-03-18  9:20                                   ` Dmitry Gutov
  2015-03-18 13:13                                 ` Stefan Monnier
  1 sibling, 1 reply; 32+ messages in thread
From: Artur Malabarba @ 2015-03-18  9:10 UTC (permalink / raw)
  To: Stefan Monnier, Daniel Colascione, Artur Malabarba, emacs-devel

>     + just there to save a funcall (ad-get-advice-info-macro,
>       shadow-cluster-name)
>       Shouldn't those be defsubsts instead of macros?

Yes, I believe they should.



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-18  9:10                                 ` Artur Malabarba
@ 2015-03-18  9:20                                   ` Dmitry Gutov
  0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Gutov @ 2015-03-18  9:20 UTC (permalink / raw)
  To: bruce.connor.am, Stefan Monnier, Daniel Colascione, emacs-devel

On 03/18/2015 11:10 AM, Artur Malabarba wrote:
>>      + just there to save a funcall (ad-get-advice-info-macro,
>>        shadow-cluster-name)
>>        Shouldn't those be defsubsts instead of macros?

Or plain functions, with compiler-macro declarations?



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-18  7:17                               ` Tassilo Horn
  2015-03-18  9:10                                 ` Artur Malabarba
@ 2015-03-18 13:13                                 ` Stefan Monnier
  2015-03-18 16:12                                   ` Tassilo Horn
  1 sibling, 1 reply; 32+ messages in thread
From: Stefan Monnier @ 2015-03-18 13:13 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: Artur Malabarba, emacs-devel

> Ok.  I just tried it anyway, and with jit-lock disabled,
> `font-lock-flush' does nothing, e.g., the buffer portion you have

I think you've just found a bug.  How did you disable jit-lock?

Can you try "emacs -Q --eval '(setq font-lock-support-mode nil)'" and
retry your test.

> If no one comes up with a (semi-)automated way of identifying
> function-like macros, I'll go check the ~1300 macros in emacs manually.

We could also do it lazily: wait for people to complain and fix those
that come up.

>     + just there to save a funcall (ad-get-advice-info-macro,
>       shadow-cluster-name)
>       Shouldn't those be defsubsts instead of macros?

Yes, but sometimes having them as macros is better (e.g., the code
should be a bit faster than with a defsubst; also macros are expanded by
gv.el so defining them as macros lets you use them as places in `setf'
and `push' as well.  The new `define-inline' does address both of those
problems, OTOH).


        Stefan



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-18 13:13                                 ` Stefan Monnier
@ 2015-03-18 16:12                                   ` Tassilo Horn
  2015-03-18 16:44                                     ` Stefan Monnier
  0 siblings, 1 reply; 32+ messages in thread
From: Tassilo Horn @ 2015-03-18 16:12 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Daniel Colascione, Artur Malabarba, emacs-devel

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> Ok.  I just tried it anyway, and with jit-lock disabled,
>> `font-lock-flush' does nothing, e.g., the buffer portion you have
>
> I think you've just found a bug.  How did you disable jit-lock?

Finding vhdl-mode.el (because that's large) and then doing M-:
(jit-lock-mode nil).

> Can you try "emacs -Q --eval '(setq font-lock-support-mode nil)'" and
> retry your test.

Yes.  So I did that and found vhdl-mode.el again.  Nothing is fontified.
(font-lock-flush) doesn't change that, but (font-lock-ensure) fontifies
the complete buffer.

>> If no one comes up with a (semi-)automated way of identifying
>> function-like macros, I'll go check the ~1300 macros in emacs
>> manually.
>
> We could also do it lazily: wait for people to complain and fix those
> that come up.

That's a good plan.  :-)

So Daniel and Drew, feel free to complain about concrete macros.  Or
just add a (declare (no-font-lock-keyword t)) to them yourself.

>>     + just there to save a funcall (ad-get-advice-info-macro,
>>       shadow-cluster-name)
>>       Shouldn't those be defsubsts instead of macros?
>
> Yes, but sometimes having them as macros is better (e.g., the code
> should be a bit faster than with a defsubst; also macros are expanded
> by gv.el so defining them as macros lets you use them as places in
> `setf' and `push' as well.  The new `define-inline' does address both
> of those problems, OTOH).

Oh, cool.  Is that going to be an alternative to `defsubst' or is the
plan to make it `defsubst' at some point in time?  (At least from a very
brief look, it seems to provide the same interface for the same
use-case.)

Bye,
Tassilo



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

* Re: [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically
  2015-03-18 16:12                                   ` Tassilo Horn
@ 2015-03-18 16:44                                     ` Stefan Monnier
  0 siblings, 0 replies; 32+ messages in thread
From: Stefan Monnier @ 2015-03-18 16:44 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: Artur Malabarba, emacs-devel

> Finding vhdl-mode.el (because that's large) and then doing M-:
> (jit-lock-mode nil).

jit-lock-mode used to be a proper minor mode, but it's not any more and
calling this `jit-lock-mode function is basically a bad idea.

>> Can you try "emacs -Q --eval '(setq font-lock-support-mode nil)'" and
>> retry your test.
> Yes.  So I did that and found vhdl-mode.el again.  Nothing is fontified.

That's because vhdl-mode.el is larger than font-lock-maximum-size.

> Oh, cool.  Is that going to be an alternative to `defsubst'

It's an alternative.

> or is the plan to make it `defsubst' at some point in time?

It's not compatible.


        Stefan



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

end of thread, other threads:[~2015-03-18 16:44 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20150315082509.21193.18465@vcs.savannah.gnu.org>
     [not found] ` <E1YX3rB-0005WV-PA@vcs.savannah.gnu.org>
2015-03-15  9:12   ` [Emacs-diffs] master 51e7e46: Font-lock elisp macros/special forms dynamically Daniel Colascione
2015-03-15 15:11     ` Artur Malabarba
2015-03-15 17:20     ` Drew Adams
2015-03-15 19:15     ` Stefan Monnier
2015-03-16  1:35       ` Artur Malabarba
2015-03-16  3:07         ` Stefan Monnier
2015-03-16  7:07           ` Tassilo Horn
2015-03-16  7:10             ` Daniel Colascione
2015-03-16  7:54               ` Tassilo Horn
2015-03-16  9:36                 ` Tassilo Horn
2015-03-16 12:58               ` Stefan Monnier
2015-03-16 14:47                 ` Tassilo Horn
2015-03-16 17:31                   ` Stefan Monnier
2015-03-16 20:26                     ` Tassilo Horn
2015-03-16 20:39                       ` Daniel Colascione
2015-03-16 20:56                         ` Stefan Monnier
2015-03-17  9:36                           ` Tassilo Horn
2015-03-17 10:17                             ` Artur Malabarba
2015-03-17 16:34                             ` Stefan Monnier
2015-03-17 16:47                               ` Nicolas Richard
2015-03-18  7:17                               ` Tassilo Horn
2015-03-18  9:10                                 ` Artur Malabarba
2015-03-18  9:20                                   ` Dmitry Gutov
2015-03-18 13:13                                 ` Stefan Monnier
2015-03-18 16:12                                   ` Tassilo Horn
2015-03-18 16:44                                     ` Stefan Monnier
2015-03-16 12:56             ` Stefan Monnier
2015-03-16  7:41       ` Tassilo Horn
2015-03-16 12:59         ` Stefan Monnier
2015-03-16 14:23           ` Artur Malabarba
2015-03-15 15:09   ` Artur Malabarba
2015-03-16  6:40     ` [Emacs-diffs] " Tassilo Horn

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).