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

[wsnyder@wsnyder.org (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