GNU bug report logs - #30235
[patch] undo-tree byte-compiler warnings

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 30235 in the body.
You can then email your comments to 30235 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Alex Branham <alex.branham <at> gmail.com>
To: "" <bug-gnu-emacs <at> gnu.org>
Subject: [patch] undo-tree byte-compiler warnings
Date: Tue, 23 Jan 2018 17:18:00 -0600

[Message part 1 (text/plain, inline)]

I'm not sure if this is the right place to submit patches for ELPA
packages, but I couldn't find anywhere else to go. Let me know if this
should go some place else.

This patch silences several byte compiler warnings, mostly having to do
with unused lexical variables in loops. There are two remaining that I
couldn't figure out how to deal with because I'm not terribly familiar
with the innards of Emacs's undo feature. (Note that the line numbers
are after applying the attached patch):

In undo-tree-pull-undo-in-region-branch:
undo-tree.el:2261:16:Warning: ‘undo-elt-crosses-region’ is an obsolete
    function (as of 25.1).

In undo-tree-pull-redo-in-region-branch:
undo-tree.el:2470:16:Warning: ‘undo-elt-crosses-region’ is an obsolete
    function (as of 25.1).


From f9a275a3cdb586f9ea91e26be84b4a42efe957fc Mon Sep 17 00:00:00 2001
From: Alex Branham <branham <at> utexas.edu>
Date: Tue, 23 Jan 2018 17:10:48 -0600
Subject: [PATCH] Silence the byte-compiler

---
 undo-tree.el | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/undo-tree.el b/undo-tree.el
index 6c7ee2f..fc52cff 100644
--- a/undo-tree.el
+++ b/undo-tree.el
@@ -757,6 +757,10 @@
 (eval-when-compile (require 'cl))
 (require 'diff)

+;; silence the byte compiler
+(defvar undo-tree-mode)
+(defvar undo-tree-visualizer-selection-mode)
+

 
 ;;; =====================================================================
@@ -2048,7 +2052,7 @@ which is defined in the `warnings' library.\n")
        ((= (mod num-children 2) 1)
         (setq p (undo-tree-node-next node))
         ;; compute left-width
-        (dotimes (i (/ num-children 2))
+        (dotimes (_i (/ num-children 2))
           (if (undo-tree-node-lwidth (car p))
               (incf lwidth (+ (undo-tree-node-lwidth (car p))
                               (undo-tree-node-cwidth (car p))
@@ -2064,7 +2068,7 @@ which is defined in the `warnings' library.\n")
         ;; compute right-width
         (incf rwidth (undo-tree-node-rwidth (car p)))
         (setq p (cdr p))
-        (dotimes (i (/ num-children 2))
+        (dotimes (_i (/ num-children 2))
           (if (undo-tree-node-lwidth (car p))
               (incf rwidth (+ (undo-tree-node-lwidth (car p))
                               (undo-tree-node-cwidth (car p))
@@ -2076,7 +2080,7 @@ which is defined in the `warnings' library.\n")
        (t
         (setq p (undo-tree-node-next node))
         ;; compute left-width
-        (dotimes (i (/ num-children 2))
+        (dotimes (_i (/ num-children 2))
           (if (undo-tree-node-lwidth (car p))
               (incf lwidth (+ (undo-tree-node-lwidth (car p))
                               (undo-tree-node-cwidth (car p))
@@ -2086,7 +2090,7 @@ which is defined in the `warnings' library.\n")
         ;; centre-width is 0 when number of children is even
         (setq cwidth 0)
         ;; compute right-width
-        (dotimes (i (/ num-children 2))
+        (dotimes (_i (/ num-children 2))
           (if (undo-tree-node-lwidth (car p))
               (incf rwidth (+ (undo-tree-node-lwidth (car p))
                               (undo-tree-node-cwidth (car p))
@@ -2748,7 +2752,7 @@ changes within the current region."
     ;; `buffer-undo-tree'
     (undo-list-transfer-to-tree)

-    (dotimes (i (or (and (numberp arg) (prefix-numeric-value arg)) 1))
+    (dotimes (_i (or (and (numberp arg) (prefix-numeric-value arg)) 1))
       ;; check if at top of undo tree
       (unless (undo-tree-node-previous (undo-tree-current buffer-undo-tree))
 	(user-error "No further undo information"))
@@ -2857,7 +2861,7 @@ changes within the current region."
     ;; `buffer-undo-tree'
     (undo-list-transfer-to-tree)

-    (dotimes (i (or (and (numberp arg) (prefix-numeric-value arg)) 1))
+    (dotimes (_i (or (and (numberp arg) (prefix-numeric-value arg)) 1))
       ;; check if at bottom of undo tree
       (when (null (undo-tree-node-next (undo-tree-current buffer-undo-tree)))
 	(user-error "No further redo information"))
@@ -3576,7 +3580,7 @@ signaling an error if file is not found."
            (car (undo-tree-node-next node)))))
       (move-marker (setq pos (make-marker)) (point))
       (setq n (cons nil (undo-tree-node-next node)))
-      (dotimes (i (/ num-children 2))
+      (dotimes (_i (/ num-children 2))
         (setq n (cdr n))
         (when (or (null active-branch)
                   (eq (car n)
@@ -3629,7 +3633,7 @@ signaling an error if file is not found."
         (move-marker pos (point)))
       ;; right subtrees
       (move-marker trunk-pos (1+ trunk-pos))
-      (dotimes (i (/ num-children 2))
+      (dotimes (_i (/ num-children 2))
         (setq n (cdr n))
         (when (or (null active-branch)
                   (eq (car n)
@@ -3684,7 +3688,7 @@ signaling an error if file is not found."
   (when (characterp str)
     (setq str (make-string arg str))
     (setq arg 1))
-  (dotimes (i arg) (insert str))
+  (dotimes (_i arg) (insert str))
   (setq arg (* arg (length str)))
   (undo-tree-move-forward arg)
   ;; make sure mark isn't active, otherwise `backward-delete-char' might
@@ -3769,7 +3773,7 @@ signaling an error if file is not found."
 	(undo-tree-move-forward
 	 (+ (undo-tree-node-char-rwidth (car n))
 	    (/ undo-tree-visualizer-spacing 2) 1))
-	(dotimes (i (- (/ l 2) p 1))
+	(dotimes (_i (- (/ l 2) p 1))
 	  (setq n (cdr n))
 	  (undo-tree-move-forward
 	   (+ (undo-tree-node-char-lwidth (car n))
@@ -3787,7 +3791,7 @@ signaling an error if file is not found."
 	   (+ (undo-tree-node-char-rwidth (car n))
 	      (/ undo-tree-visualizer-spacing 2) 1))
 	  (setq n (cdr n)))
-	(dotimes (i (- p (/ l 2) (mod l 2)))
+	(dotimes (_i (- p (/ l 2) (mod l 2)))
 	  (undo-tree-move-backward
 	   (+ (undo-tree-node-char-lwidth (car n))
 	      (undo-tree-node-char-rwidth (car n))
@@ -3801,13 +3805,13 @@ signaling an error if file is not found."


 (defun undo-tree-timestamp-to-string
-  (timestamp &optional relative current register)
+    (timestamp &optional relative current register)
   ;; Convert TIMESTAMP to string (either absolute or RELATVE time), indicating
   ;; if it's the CURRENT node and/or has an associated REGISTER.
   (if relative
       ;; relative time
       (let ((time (floor (float-time
-			  (subtract-time (current-time) timestamp))))
+			  (time-subtract (current-time) timestamp))))
 	    n)
 	(setq time
 	      ;; years
@@ -4232,7 +4236,7 @@ specifies `saved', and a negative prefix argument specifies
     (user-error "Undo-tree mode not enabled in buffer"))
   (let ((node undo-tree-visualizer-selected-node))
     (catch 'top
-      (dotimes (i (or arg 1))
+      (dotimes (_i (or arg 1))
 	(unless (undo-tree-node-previous node) (throw 'top t))
 	(setq node (undo-tree-node-previous node))))
     ;; when using lazy drawing, extend tree upwards as required
@@ -4254,7 +4258,7 @@ specifies `saved', and a negative prefix argument specifies
     (user-error "Undo-tree mode not enabled in buffer"))
   (let ((node undo-tree-visualizer-selected-node))
     (catch 'bottom
-      (dotimes (i (or arg 1))
+      (dotimes (_i (or arg 1))
 	(unless (nth (undo-tree-node-branch node) (undo-tree-node-next node))
 	  (throw 'bottom t))
 	(setq node
@@ -4281,7 +4285,7 @@ specifies `saved', and a negative prefix argument specifies
     (goto-char (undo-tree-node-marker undo-tree-visualizer-selected-node))
     (setq end (line-end-position))
     (catch 'end
-      (dotimes (i arg)
+      (dotimes (_i arg)
 	(while (or (null node) (eq node undo-tree-visualizer-selected-node))
 	  (forward-char)
 	  (setq node (get-text-property (point) 'undo-tree-node))
@@ -4304,7 +4308,7 @@ specifies `saved', and a negative prefix argument specifies
     (goto-char (undo-tree-node-marker undo-tree-visualizer-selected-node))
     (setq beg (line-beginning-position))
     (catch 'beg
-      (dotimes (i arg)
+      (dotimes (_i arg)
 	(while (or (null node) (eq node undo-tree-visualizer-selected-node))
 	  (backward-char)
 	  (setq node (get-text-property (point) 'undo-tree-node))
--
2.16.1

[0001-Silence-the-byte-compiler.patch (text/x-patch, attachment)]

Message #10 received at 30235 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Alex Branham <alex.branham <at> gmail.com>
Cc: 30235 <at> debbugs.gnu.org, 24354 <at> debbugs.gnu.org
Subject: Re: bug#30235: [patch] undo-tree byte-compiler warnings
Date: Tue, 25 Jun 2019 13:51:14 +0200

Alex Branham <alex.branham <at> gmail.com> writes:

> I'm not sure if this is the right place to submit patches for ELPA
> packages, but I couldn't find anywhere else to go. Let me know if this
> should go some place else.
>
> This patch silences several byte compiler warnings, mostly having to do
> with unused lexical variables in loops.

The unused dolist fixes look OK, but they could also be rewritten as

(cl-loop repeat ...
         do ...)

to avoid the binding altogether.

The other changes (time-subtract/subtract-time) I'm not so sure about,
because ELPA things are supposed to work across a large number of Emacs
versions, and time-subtract is newer than subtract-time.

The same with the other obsolete functions -- you're bound to get those
warnings in ELPA packages.

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

Message #13 received at 30235 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Alex Branham <alex.branham <at> gmail.com>, 30235 <at> debbugs.gnu.org
Subject: Re: bug#30235: [patch] undo-tree byte-compiler warnings
Date: Tue, 25 Jun 2019 14:32:14 +0100

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Alex Branham <alex.branham <at> gmail.com> writes:
>
>> I'm not sure if this is the right place to submit patches for ELPA
>> packages, but I couldn't find anywhere else to go. Let me know if this
>> should go some place else.
>>
>> This patch silences several byte compiler warnings, mostly having to do
>> with unused lexical variables in loops.
>
> The unused dolist fixes look OK, but they could also be rewritten as
>
> (cl-loop repeat ...
>          do ...)
>
> to avoid the binding altogether.

There's really no need to completely rewrite all those dotimes in terms
of cl-loop.  Writing (dotimes (_ ...) ...) is quite common and fine.

> The other changes (time-subtract/subtract-time) I'm not so sure about,
> because ELPA things are supposed to work across a large number of Emacs
> versions, and time-subtract is newer than subtract-time.

Sure, but subtract-time has been called time-subtract for almost two
decades now[1].

[1: 74fcda73dd]: Add autoload cookies.  Many doc fixes.
  2002-01-27 23:30:29 +0000
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=74fcda73dd8743f66256aa5fdaff6260dc356c54

> The same with the other obsolete functions -- you're bound to get those
> warnings in ELPA packages.

Sure, but often a compatibility shim can go a long way, and in this case
the question is why was undo-elt-crosses-region obsoleted as part of
bug#17235, and what should undo-tree.el do in future Emacs versions.

Maybe Barry, Toby, or Stefan should be CCed in this or a new discussion
about it, seeing as Jonas' concerns were never addressed, AFAICT.

Thanks,

-- 
Basil

Message #16 received at 30235 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: Alex Branham <alex.branham <at> gmail.com>, 30235 <at> debbugs.gnu.org
Subject: Re: bug#30235: [patch] undo-tree byte-compiler warnings
Date: Tue, 25 Jun 2019 15:34:58 +0200

"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> Sure, but subtract-time has been called time-subtract for almost two
> decades now[1].

That sounds safe, then.  :-)

> Maybe Barry, Toby, or Stefan should be CCed in this or a new discussion
> about it, seeing as Jonas' concerns were never addressed, AFAICT.

Sure; go ahead and poke them about it.

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

Message #19 received at 30235 <at> debbugs.gnu.org (full text, mbox):

From: Richard Stallman <rms <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: alex.branham <at> gmail.com, 30235 <at> debbugs.gnu.org, 24354 <at> debbugs.gnu.org
Subject: Re: bug#30235: [patch] undo-tree byte-compiler warnings
Date: Tue, 25 Jun 2019 23:24:32 -0400

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > The unused dolist fixes look OK, but they could also be rewritten as

  > (cl-loop repeat ...
  >          do ...)

  > to avoid the binding altogether.

Warnings are meant to be our helpers, not our masters.
We should not get in the habit of jumping through hoops
to satisfy a demanding, rigid program.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Message #22 received at 30235 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Alex Branham <alex.branham <at> gmail.com>
Cc: 30235 <at> debbugs.gnu.org, toby-undo-tree <at> dr-qubit.org
Subject: Re: bug#30235: [patch] undo-tree byte-compiler warnings
Date: Tue, 17 Sep 2019 14:39:04 +0200

Alex Branham <alex.branham <at> gmail.com> writes:

> -      (dotimes (i arg)
> +      (dotimes (_i arg)

Jonas said:

> Toby, I am aware that I have suggested before that you use `_' as VAR in
> calls to `dotimes' instead of `i'.  You rejected that suggestion,
> claiming that the byte-compiler complaining about `i' not being used is
> a bug.

While my preference is to avoid compiler warnings (because having a
warning-free build allows anybody working on the code to proceed with
greater speed and confidence), I think that for ELPA code this should be
up to the maintainer.  Since it seems like Toby is opposed to these
changes, I'm closing this bug report.

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

Message #29 received at 30235 <at> debbugs.gnu.org (full text, mbox):

From: Toby Cubitt <toby-dated-1570017117.01e866 <at> dr-qubit.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>, Alex Branham <alex.branham <at> gmail.com>
Cc: 30235 <at> debbugs.gnu.org, toby-undo-tree <at> dr-qubit.org
Subject: Re: bug#30235: [patch] undo-tree byte-compiler warnings
Date: Wed, 18 Sep 2019 12:51:46 +0100

Hi Alex,

Not opposed to this at all! (I forgot what i said previously, but if I was then it was before I knew of the _ method.) Indeed, I think I already made exactly this change a while back in the undo-tree development repo, but maybe forgot to transfer this into ELPA.

Undo-tree went into ELPA before there was support for separate per-package git repos in ELPA, which is why this isn't automatic. It makes upstreaming changes from my development repo(s) to ELPA that bit more complicated than a simple git push. I'm very busy with real life these days, so this inevitably gets forgotten/postponed. Maybe this is a good opportunity to shift all my ELPA packages over to per-package git repos, so I can just make the ELPA repos upstreams of my package development repos?

Best wishes,
Toby 

On 17 September 2019 13:39:04 BST, Lars Ingebrigtsen <larsi <at> gnus.org> wrote:
>Alex Branham <alex.branham <at> gmail.com> writes:
>
>> -      (dotimes (i arg)
>> +      (dotimes (_i arg)
>
>Jonas said:
>
>> Toby, I am aware that I have suggested before that you use `_' as VAR
>in
>> calls to `dotimes' instead of `i'.  You rejected that suggestion,
>> claiming that the byte-compiler complaining about `i' not being used
>is
>> a bug.
>
>While my preference is to avoid compiler warnings (because having a
>warning-free build allows anybody working on the code to proceed with
>greater speed and confidence), I think that for ELPA code this should
>be
>up to the maintainer.  Since it seems like Toby is opposed to these
>changes, I'm closing this bug report.

-- 
Dr T. S. Cubitt
Reader (Associate Professor) in Quantum Information
Royal Society University Research Fellow
Department of Computer Science
University College London

email: tsc25 <at> cantab.net 
web:   www.dr-qubit.org

Message #32 received at 30235 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Toby Cubitt <tsc25 <at> cantab.net>
Cc: Alex Branham <alex.branham <at> gmail.com>, 30235 <at> debbugs.gnu.org,
 toby-undo-tree <at> dr-qubit.org
Subject: Re: bug#30235: [patch] undo-tree byte-compiler warnings
Date: Wed, 18 Sep 2019 15:53:49 +0200

Toby Cubitt <tsc25 <at> cantab.net> writes:

> Not opposed to this at all! (I forgot what i said previously, but if I
> was then it was before I knew of the _ method.) Indeed, I think I
> already made exactly this change a while back in the undo-tree
> development repo, but maybe forgot to transfer this into ELPA.

Oh, OK.  :-)  

> Undo-tree went into ELPA before there was support for separate
> per-package git repos in ELPA, which is why this isn't automatic. It
> makes upstreaming changes from my development repo(s) to ELPA that bit
> more complicated than a simple git push. I'm very busy with real life
> these days, so this inevitably gets forgotten/postponed. Maybe this is
> a good opportunity to shift all my ELPA packages over to per-package
> git repos, so I can just make the ELPA repos upstreams of my package
> development repos?

Sounds like a good idea.

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.