Re: Get rid of verilog-no-change-functions

Re: Get rid of verilog-no-change-functions

[Wilson Snyder]

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

Re: Get rid of verilog-no-change-functions

[Stefan Monnier]

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

Perfect, no hurry, thanks for taking care of it.


        Stefan

Re: Get rid of verilog-no-change-functions

[Wilson Snyder]

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

Re: Get rid of verilog-no-change-functions

[Stefan Monnier]

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

Re: Get rid of verilog-no-change-functions

[Andreas Schwab]

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

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

eval-when-compile doesn't introduce a scope.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Get rid of verilog-no-change-functions

[Stefan Monnier]

> eval-when-compile doesn't introduce a scope.

Depends on your definition of scope.  And I'm pretty sure your notion of
scope doesn't exactly match the currently implemented notion of scope,
so better not depend too much on such corner cases.

And yes, the "natural" implementation of eval-when-compile where the
compiler just passes the expression to `eval' would end up introducing
a new scope and (eval-when-compile (defvar foo)) would end up not having
any effect at all.

Been there done that!  And then took me some time to figure out why the
existing code happens to work as people expect it.  It really is
an accident that derives from a "too lax" implementation of the compiler.


        Stefan

Re: Get rid of verilog-no-change-functions

[Stefan Monnier]

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

Re: Get rid of verilog-no-change-functions

[Wilson Snyder]

>> +    (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

Re: Get rid of verilog-no-change-functions

[Stefan Monnier]

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

Good, thanks.

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

Ha!  Indeed, the XEmacs byte compiler does warn about this!
Can't believe it took me so many years to find out.

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

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.

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

How 'bout the patch below?


        Stefan


diff --git a/lisp/progmodes/verilog-mode.el b/lisp/progmodes/verilog-mode.el
index f83c676..f2505e3 100644
--- a/lisp/progmodes/verilog-mode.el
+++ b/lisp/progmodes/verilog-mode.el
@@ -229,11 +229,6 @@ STRING should be given if the last search was by `string-match' on STRING."
       (defmacro customize-group (var &rest _args)
         `(customize ,var))
       )
-
-    (unless (boundp 'inhibit-point-motion-hooks)
-      (defvar inhibit-point-motion-hooks nil))
-    (unless (boundp 'deactivate-mark)
-      (defvar deactivate-mark nil))
     )
   ;;
   ;; OK, do this stuff if we are NOT XEmacs:
@@ -243,6 +238,10 @@ STRING should be given if the last search was by `string-match' on STRING."
 	`(and transient-mark-mode mark-active))))
   )
 
+(defvar inhibit-point-motion-hooks)
+(defvar inhibit-modification-hooks)
+(defvar deactivate-mark)
+
 ;; Provide a regular expression optimization routine, using regexp-opt
 ;; if provided by the user's elisp libraries
 (eval-and-compile
@@ -3231,8 +3230,7 @@ user-visible changes to the buffer must not be within a
 	  (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
@@ -3240,16 +3238,23 @@ user-visible changes to the buffer must not be within a
 	 (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))))))
 
 (defmacro verilog-save-no-change-functions (&rest body)
   "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)
-     (progn ,@body)))
+  ;; For use when we're about to make many changes over the whole buffer,
+  ;; so we avoid running the *-change-functions too many times.
+  (let ((lensym (make-symbol "length")))
+    `(let* ((inhibit-point-motion-hooks t)
+            (verilog-no-change-functions t)
+            (inhibit-modification-hooks t)
+            (,lensym (- (point-max) (point-min))))
+       (run-hook-with-args 'before-change-functions (point-min) (point-max))
+       ,@body
+       (run-hook-with-args 'after-change-functions (point-min) (point-max) ,lensym))))
 
 (defvar verilog-save-font-mod-hooked nil
   "Local variable when inside a `verilog-save-font-mods' block.")
@@ -9956,9 +9961,9 @@ Cache the output of function so next call may have faster access."
 	    (t
 	     ;; Read from file
 	     ;; Clear then restore any highlighting to make emacs19 happy
-	     (let (func-returns)
-	       (verilog-save-font-mods
-		(setq func-returns (funcall function)))
+	     (let ((func-returns
+                    (verilog-save-font-mods ;FIXME: Is it still needed?
+                     (funcall function))))
 	       ;; Cache for next time
 	       (setq verilog-modi-cache-list
 		     (cons (list (list modi function)
@@ -13438,7 +13443,7 @@ Enable with `verilog-auto-template-warn-unused'."
 ;;; Auto top level:
 ;;
 
-(defun verilog-auto (&optional inject)  ; Use verilog-inject-auto instead of passing an arg
+(defun verilog-auto (&optional inject) ; Use verilog-inject-auto instead of passing an arg
   "Expand AUTO statements.
 Look for any /*AUTO...*/ commands in the code, as used in
 instantiations or argument headers.  Update the list of signals
@@ -13514,7 +13514,6 @@
   (unless noninteractive (message "Updating AUTOs..."))
   (if (fboundp 'dinotrace-unannotate-all)
       (dinotrace-unannotate-all))
-  (verilog-save-font-mods
    (let ((oldbuf (if (not (buffer-modified-p))
 		     (buffer-string)))
 	 (case-fold-search verilog-case-fold)
@@ -13524,7 +13523,6 @@
 	 (verilog-modi-cache-current-enable t)
 	 (verilog-modi-cache-current-max (point-min)) ; IE it's invalid
 	 verilog-modi-cache-current)
-     (unwind-protect
 	 ;; Disable change hooks for speed
 	 ;; This let can't be part of above let; must restore
 	 ;; after-change-functions before font-lock resumes
@@ -13625,9 +13623,7 @@
 		   (t (unless noninteractive (message "Updating AUTOs...done"))))
 	     ;; End of after-change protection
 	     )))
-       ;; Unwind forms
-       ;; Currently handled in verilog-save-font-mods
-       ))))
+    ))
 \f
 ;;; Skeletons:
 ;;

Re: Get rid of verilog-no-change-functions

[Wilson Snyder]

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

Re: Get rid of verilog-no-change-functions

[Stefan Monnier]

> 3. This code was originally developed on Emacs 21 so it's
> odd that inhibit-modification-hooks wasn't sufficient.

inhibit-modification-hooks was new in Emacs-21 and not widely known, so
I'm really not surprised.  Even nowadays it's still common to see people
let-binding after-change-functions instead of inhibit-modification-hooks.

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

AFAIK my patch should work just as well at least for the
verilog-save-buffer-state part since XEmacs doesn't run
after-change-functions for text-property modifications.

For verilog-save-no-change-functions, I don't understand what is the
intention behind preventing after-change-functions (it intuitively seems
wrong, so I'm obviously missing something).

> +    (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.

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.

> @@ -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

Here it's important to remove before-change-functions and
after-change-functions.

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


        Stefan

Get rid of verilog-no-change-functions

[Stefan Monnier]

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