unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Re: Get rid of verilog-no-change-functions
@ 2015-09-14 21:09 Wilson Snyder
  2015-09-15 13:45 ` Stefan Monnier
  0 siblings, 1 reply; 12+ messages in thread
From: Wilson Snyder @ 2015-09-14 21:09 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel


>> +    (unless (boundp 'inhibit-modification-hooks)
>> +      (defvar inhibit-modification-hooks nil))
>
>No, no, no, no!
>Just use
>
>   (defvar inhibit-modification-hooks)
>
>if you want to silence the byte-compiler.

You're right of course, we use defvar everywhere else; not
sure why the older code there did that.  Fixed all three.

>But AFAIK as long you don't switch to "lexical-binding:t", no
>byte-compiler will complain if you let-bind an unknown variable without
>using it, so even that (defvar inhibit-modification-hooks) is
>not indispensable.

The default byte-compile warns. We always want to be as
warning free as possible, even on XEmacs.

>> @@ -3247,6 +3250,7 @@ user-visible changes to the buffer must not be within a
>>  For insignificant changes, see instead `verilog-save-buffer-state'."
>>    `(let* ((inhibit-point-motion-hooks t)
>> +	  (inhibit-modification-hooks t)
>>  	  (verilog-no-change-functions t)
>>  	  before-change-functions
>>  	  after-change-functions)
>>       (progn ,@body)))
>
>As mentioned, here I'd have to understand why we need to prevent
>*-change-functions from running.  After all, this is used in places
>where we make very significant changes to the buffer and where font-lock
>(for instance) would want to know about it.

The child functions perform hundreds of thousands of minor
changes, then call the hooks on everything.  Doing it this
way makes an update maybe 10 seconds versus (as it once did)
many minutes - which was almost unusable. At the end of
everything fontification is correct.

New patch below - if OK would you like to apply or shall I?


diff --git a/verilog-mode.el b/verilog-mode.el
index 57063b5..97eedd3 100644
--- a/verilog-mode.el
+++ b/verilog-mode.el
@@ -230,10 +230,9 @@ STRING should be given if the last search was by `string-match' on STRING."
         `(customize ,var))
       )
 
-    (unless (boundp 'inhibit-point-motion-hooks)
-      (defvar inhibit-point-motion-hooks nil))
-    (unless (boundp 'deactivate-mark)
-      (defvar deactivate-mark nil))
+    (defvar inhibit-modification-hooks)
+    (defvar inhibit-point-motion-hooks)
+    (defvar deactivate-mark)
     )
   ;;
   ;; OK, do this stuff if we are NOT XEmacs:
@@ -3230,9 +3229,8 @@ user-visible changes to the buffer must not be within a
 	  (buffer-undo-list t)
 	  (inhibit-read-only t)
 	  (inhibit-point-motion-hooks t)
+	  (inhibit-modification-hooks t)
 	  (verilog-no-change-functions t)
-	  before-change-functions
-	  after-change-functions
 	  deactivate-mark
 	  buffer-file-name ; Prevent primitives checking
 	  buffer-file-truename)	; for file modification
@@ -3246,9 +3244,8 @@ user-visible changes to the buffer must not be within a
   "Execute BODY forms, disabling all change hooks in BODY.
 For insignificant changes, see instead `verilog-save-buffer-state'."
   `(let* ((inhibit-point-motion-hooks t)
-	  (verilog-no-change-functions t)
-	  before-change-functions
-	  after-change-functions)
+	  (inhibit-modification-hooks t)
+	  (verilog-no-change-functions t))
      (progn ,@body)))
 
 (defvar verilog-save-font-mod-hooked nil



^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: Get rid of verilog-no-change-functions
@ 2015-10-29 13:48 Wilson Snyder
  2015-10-29 15:31 ` Stefan Monnier
  0 siblings, 1 reply; 12+ messages in thread
From: Wilson Snyder @ 2015-10-29 13:48 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel


I have this on a branch awaiting testing on older Emacsen (21 etc).  I'll get that done and applied into Emacs trunk.

-Wilson


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

>Hi Wilson,
>
>Did (or will) you install a patch along the lines fleshed out in
>this thread?
>If you prefer I can do it on the Emacs side instead, but I'd rather you
>do it, since you're in a better position to make sure it actually works.
>
>
>        Stefan
>
>
>>>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>>> Also, I see that verilog-save-no-change-functions is wrapped inside
>>>> verilog-save-font-mods in verilog-auto, but not in verilog-delete-auto.
>
>>> The common use of delete-auto is under verilog-auto itself,
>>> so if we added it to delete-auto we'd be calling the hooks
>>> at both auto's exiting of verilog-delete-auto and at the
>>> exit of verilog-auto itself.
>
>> `verilog-delete-auto' is an interactive function, so we do want to
>> handle that case as well.
>
>>> We'd then be better off pulling the guts out of
>>> verilog-delete-auto (without
>>> verilog-save-no-change-functions) and call those guts from
>>> verilog-auto and verilog-delete-auto.
>
>> Indeed, that would be to right thing to do, I think.
>
>>> But anyhow I've never heard complaints of verilog-delete-auto being
>>> slow as it makes an order-of-magnitude fewer changes, so doesn't seem
>>> worth the work.
>
>> You mean we could remove verilog-save-no-change-functions from it?
>> If you say so, that's fine by me.
>
>>> Also why do you suggest a defvar working would be an "accident"?
>>> These defvars only needs to exist when compiling.
>
>> *eval*uating (defvar foo) has no effect, other than to declare that var
>> to be dynamically scoped *in that scope*.  E.g.
>
>>    (defun bar ()
>>      (defvar foo)
>>      ...)
>
>> make `foo' be dynamically scoped in that scope.  So
>
>>    (eval-when-compile
>>      (defvar foo)
>>      ...)
>
>> Would most logically make `foo' be dynamically scoped within the
>> eval-when-compile but not outside of it.
>
>> The only reason why it works is an implementation accident:
>> eval-when-compile (when run from the byte-compiler) first compiles its
>> body, and that has the side-effect that it ends up declaring `foo' also
>> outside of the eval-when-compile.  It also has a few other side-effect,
>> and like this one, some of them are halfway between bugs and features.
>
>>>> (progn ,@body)
>>>> (and (not modified)
>>>> (buffer-modified-p)
>>>> -	    (set-buffer-modified-p nil)))))
>>>> +            (if (fboundp 'restore-buffer-modified-p)
>>>> +                (restore-buffer-modified-p nil)
>>>> +              (set-buffer-modified-p nil))))))
>>> Can you explain why restore-buffer-modified-p is preferred?
>
>> Because it avoids forcing a recomputation of the mode-line.
>
>>> The documentation suggests this may be suspicious.
>
>> But in the present case, restore-buffer-modified-p would indeed
>> restore the buffer-modified-p state, thus there's no need to recompute
>> the mode-line.
>
>> This was introduced specifically for this kind of use.  See for example
>> the definition of with-silent-modifications.
>
>
>>         Stefan



^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: Get rid of verilog-no-change-functions
@ 2015-09-15 23:51 Wilson Snyder
  2015-09-16  1:05 ` Stefan Monnier
  0 siblings, 1 reply; 12+ messages in thread
From: Wilson Snyder @ 2015-09-15 23:51 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel


>>> As mentioned, here I'd have to understand why we need to prevent
>>> *-change-functions from running.  After all, this is used in places
>>> where we make very significant changes to the buffer and where font-lock
>>> (for instance) would want to know about it.
>
>> The child functions perform hundreds of thousands of minor
>> changes, then call the hooks on everything.  Doing it this
>> way makes an update maybe 10 seconds versus (as it once did)
>> many minutes - which was almost unusable. At the end of
>> everything fontification is correct.
>
>Combining all the *-change-functions calls makes a lot of sense (that's
>why we have combine-after-change-calls, for example).  But I don't see
>where you "then call the hooks on everything".
>
>I see the verilog-save-font-mods which re-font-locks the buffer
>afterwards, but since it disables font-lock at the beginning it means
>that verilog-save-no-change-functions is not needed to prevent
>font-lock hooks.
>
>And of course, disabling/enabling font-lock-mode doesn't take care of
>other users of *-change-functions.

Your patches make sense, and there's no appreciable performance loss.

>Also, I see that verilog-save-no-change-functions is wrapped inside
>verilog-save-font-mods in verilog-auto, but not in verilog-delete-auto.

The common use of delete-auto is under verilog-auto itself,
so if we added it to delete-auto we'd be calling the hooks
at both auto's exiting of verilog-delete-auto and at the
exit of verilog-auto itself.

We'd then be better off pulling the guts out of
verilog-delete-auto (without
verilog-save-no-change-functions) and call those guts from
verilog-auto and verilog-delete-auto.

But anyhow I've never heard complaints of
verilog-delete-auto being slow as it makes an
order-of-magnitude fewer changes, so doesn't seem worth the
work.


>> -    (unless (boundp 'inhibit-point-motion-hooks)
>> -      (defvar inhibit-point-motion-hooks nil))
>> -    (unless (boundp 'deactivate-mark)
>> -      (defvar deactivate-mark nil))
>> +    (defvar inhibit-modification-hooks)
>> +    (defvar inhibit-point-motion-hooks)
>> +    (defvar deactivate-mark)
>
>Those defvars should not be with an eval-when-compile (they happen to
>also work if they are, currently, but that's a mere accident).

These defvars only apply to XEmacs.  I considered doing it
this way a feature, as if e.g. GNU removed one of those
variables in the future the warnings would return.  Likewise
if someone sends me a bug report showing the warnings on GNU
I want to know that those variables aren't in their version.

Also why do you suggest a defvar working would be an
"accident"?  These defvars only needs to exist when
compiling.


> 	 (progn ,@body)
>        (and (not modified)
> 	    (buffer-modified-p)
>-	    (set-buffer-modified-p nil)))))
>+            (if (fboundp 'restore-buffer-modified-p)
>+                (restore-buffer-modified-p nil)
>+              (set-buffer-modified-p nil))))))

Can you explain why restore-buffer-modified-p is preferred?
The documentation suggests this may be suspicious.

   Like `set-buffer-modified-p', with a difference
   concerning redisplay.  It is not ensured that mode lines
   will be updated to show the modified state of the current
   buffer.  Use with care.




^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: Get rid of verilog-no-change-functions
@ 2015-09-12 11:33 Wilson Snyder
  2015-09-12 20:21 ` Stefan Monnier
  0 siblings, 1 reply; 12+ messages in thread
From: Wilson Snyder @ 2015-09-12 11:33 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel


Thanks for the proposed verilog-mode.el patch.

1. Minorly, the assertion code requires verilog-in-hooks
remain in place, otherwise it fires inappropriately.

2. There is another block of similar code which presumably
we would want to fix also.

3. This code was originally developed on Emacs 21 so it's
odd that inhibit-modification-hooks wasn't sufficient. But
the the verilog-mode.el code also works on ancient XEmacs.
This is annoying for our code, but we still get bug reports
occasionally so are reluctant to abandon it.  At a minimum
inhibit-modification-hooks needs to be defined to prevent
XEmacs compile warnings.


I'd propose the below which adds inhibit-modification-hooks,
but retains the older code.  If you feel that we should
remove clearing before/after-change-functions we can do
that, which will harm XEmacs and pre-Emacs 21 performance,
but that does pass our regressions and I don't see XEmacs
performance drop as a big concern.

-Wilson

diff --git a/verilog-mode.el b/verilog-mode.el
index 57063b5..83246d6 100644
--- a/verilog-mode.el
+++ b/verilog-mode.el
@@ -230,6 +230,8 @@ STRING should be given if the last search was by `string-match' on STRING."
         `(customize ,var))
       )
 
+    (unless (boundp 'inhibit-modification-hooks)
+      (defvar inhibit-modification-hooks nil))
     (unless (boundp 'inhibit-point-motion-hooks)
       (defvar inhibit-point-motion-hooks nil))
     (unless (boundp 'deactivate-mark)
@@ -3231,6 +3233,7 @@ user-visible changes to the buffer must not be within a
 	  (inhibit-read-only t)
 	  (inhibit-point-motion-hooks t)
+	  (inhibit-modification-hooks t)  ; Emacs 21+
 	  (verilog-no-change-functions t)
 	  before-change-functions
 	  after-change-functions
 	  deactivate-mark
@@ -3247,6 +3250,7 @@ user-visible changes to the buffer must not be within a
 For insignificant changes, see instead `verilog-save-buffer-state'."
   `(let* ((inhibit-point-motion-hooks t)
+	  (inhibit-modification-hooks t)
 	  (verilog-no-change-functions t)
 	  before-change-functions
 	  after-change-functions)
      (progn ,@body)))

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

>I believe the patch below replaces a workaround with an actual fix.
>
>It's indeed unsafe to call syntax-ppss when before-change-functions is
>let-bound, but the patch avoids the problem by not let-binding
>before-change-functions, relying on the cleaner
>inhibit-modification-hooks, which was introduced way back in Emacs-21
>for similar reasons.
>
>Any objection to my applying this to Emacs's version of verilog-mode.el?
>
>
>        Stefan
>
>
>diff --git a/lisp/progmodes/verilog-mode.el b/lisp/progmodes/verilog-mode.el
>index 5fcdba6..0f90d60 100644
>--- a/lisp/progmodes/verilog-mode.el
>+++ b/lisp/progmodes/verilog-mode.el
>@@ -408,10 +408,6 @@ This function may be removed when Emacs 21 is no longer supported."
> 	    ;; And GNU Emacs 22 has obsoleted last-command-char
> 	    last-command-event)))
> 
>-(defvar verilog-no-change-functions nil
>-  "True if `after-change-functions' is disabled.
>-Use of `syntax-ppss' may break, as ppss's cache may get corrupted.")
>-
> (defvar verilog-in-hooks nil
>   "True when within a `verilog-run-hooks' block.")
> 
>@@ -422,14 +418,13 @@ Set `verilog-in-hooks' during this time, to assist AUTO caches."
>      (run-hooks ,@hooks)))
> 
> (defun verilog-syntax-ppss (&optional pos)
>-  (when verilog-no-change-functions
>-    (if verilog-in-hooks
>-	(verilog-scan-cache-flush)
>-      ;; else don't let the AUTO code itself get away with flushing the cache,
>-      ;; as that'll make things very slow
>-      (backtrace)
>-      (error "%s: Internal problem; use of syntax-ppss when cache may be corrupt"
>-	     (verilog-point-text))))
>+  (if verilog-in-hooks
>+      (verilog-scan-cache-flush)
>+    ;; else don't let the AUTO code itself get away with flushing the cache,
>+    ;; as that'll make things very slow
>+    (backtrace)
>+    (error "%s: Internal problem; use of syntax-ppss when cache may be corrupt"
>+           (verilog-point-text)))
>   (if (fboundp 'syntax-ppss)
>       (syntax-ppss pos)
>     (parse-partial-sexp (point-min) (or pos (point)))))
>@@ -3230,9 +3225,7 @@ user-visible changes to the buffer must not be within a
> 	  (buffer-undo-list t)
> 	  (inhibit-read-only t)
> 	  (inhibit-point-motion-hooks t)
>-	  (verilog-no-change-functions t)
>-	  before-change-functions
>-	  after-change-functions
>+	  (inhibit-modification-hooks t)
> 	  deactivate-mark
> 	  buffer-file-name ; Prevent primitives checking
> 	  buffer-file-truename)	; for file modification
>@@ -3246,9 +3239,7 @@ user-visible changes to the buffer must not be within a
>   "Execute BODY forms, disabling all change hooks in BODY.
> For insignificant changes, see instead `verilog-save-buffer-state'."
>   `(let* ((inhibit-point-motion-hooks t)
>-	  (verilog-no-change-functions t)
>-	  before-change-functions
>-	  after-change-functions)
>+	  (inhibit-modification-hooks t))
>      (progn ,@body)))
> 
> (defvar verilog-save-font-mod-hooked nil



^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Get rid of verilog-no-change-functions
@ 2015-09-12  4:22 Stefan Monnier
  0 siblings, 0 replies; 12+ messages in thread
From: Stefan Monnier @ 2015-09-12  4:22 UTC (permalink / raw)
  To: Wilson Snyder; +Cc: emacs-devel

I believe the patch below replaces a workaround with an actual fix.

It's indeed unsafe to call syntax-ppss when before-change-functions is
let-bound, but the patch avoids the problem by not let-binding
before-change-functions, relying on the cleaner
inhibit-modification-hooks, which was introduced way back in Emacs-21
for similar reasons.

Any objection to my applying this to Emacs's version of verilog-mode.el?


        Stefan


diff --git a/lisp/progmodes/verilog-mode.el b/lisp/progmodes/verilog-mode.el
index 5fcdba6..0f90d60 100644
--- a/lisp/progmodes/verilog-mode.el
+++ b/lisp/progmodes/verilog-mode.el
@@ -408,10 +408,6 @@ This function may be removed when Emacs 21 is no longer supported."
 	    ;; And GNU Emacs 22 has obsoleted last-command-char
 	    last-command-event)))
 
-(defvar verilog-no-change-functions nil
-  "True if `after-change-functions' is disabled.
-Use of `syntax-ppss' may break, as ppss's cache may get corrupted.")
-
 (defvar verilog-in-hooks nil
   "True when within a `verilog-run-hooks' block.")
 
@@ -422,14 +418,13 @@ Set `verilog-in-hooks' during this time, to assist AUTO caches."
      (run-hooks ,@hooks)))
 
 (defun verilog-syntax-ppss (&optional pos)
-  (when verilog-no-change-functions
-    (if verilog-in-hooks
-	(verilog-scan-cache-flush)
-      ;; else don't let the AUTO code itself get away with flushing the cache,
-      ;; as that'll make things very slow
-      (backtrace)
-      (error "%s: Internal problem; use of syntax-ppss when cache may be corrupt"
-	     (verilog-point-text))))
+  (if verilog-in-hooks
+      (verilog-scan-cache-flush)
+    ;; else don't let the AUTO code itself get away with flushing the cache,
+    ;; as that'll make things very slow
+    (backtrace)
+    (error "%s: Internal problem; use of syntax-ppss when cache may be corrupt"
+           (verilog-point-text)))
   (if (fboundp 'syntax-ppss)
       (syntax-ppss pos)
     (parse-partial-sexp (point-min) (or pos (point)))))
@@ -3230,9 +3225,7 @@ user-visible changes to the buffer must not be within a
 	  (buffer-undo-list t)
 	  (inhibit-read-only t)
 	  (inhibit-point-motion-hooks t)
-	  (verilog-no-change-functions t)
-	  before-change-functions
-	  after-change-functions
+	  (inhibit-modification-hooks t)
 	  deactivate-mark
 	  buffer-file-name ; Prevent primitives checking
 	  buffer-file-truename)	; for file modification
@@ -3246,9 +3239,7 @@ user-visible changes to the buffer must not be within a
   "Execute BODY forms, disabling all change hooks in BODY.
 For insignificant changes, see instead `verilog-save-buffer-state'."
   `(let* ((inhibit-point-motion-hooks t)
-	  (verilog-no-change-functions t)
-	  before-change-functions
-	  after-change-functions)
+	  (inhibit-modification-hooks t))
      (progn ,@body)))
 
 (defvar verilog-save-font-mod-hooked nil



^ permalink raw reply related	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-10-29 15:31 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-14 21:09 Get rid of verilog-no-change-functions Wilson Snyder
2015-09-15 13:45 ` Stefan Monnier
  -- strict thread matches above, loose matches on Subject: below --
2015-10-29 13:48 Wilson Snyder
2015-10-29 15:31 ` Stefan Monnier
2015-09-15 23:51 Wilson Snyder
2015-09-16  1:05 ` Stefan Monnier
2015-09-16  7:40   ` Andreas Schwab
2015-09-16 13:12     ` Stefan Monnier
2015-10-29 13:22   ` Stefan Monnier
2015-09-12 11:33 Wilson Snyder
2015-09-12 20:21 ` Stefan Monnier
2015-09-12  4:22 Stefan Monnier

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