Re: [elpa] externals/ebdb f7b50402cf: Final round of compiler quieting

[Stefan Monnier <monnier@iro.umontreal.ca>]

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

>     Not all the way there, but I don't know what to do about the format
>     complaints.

The suspect that problem is that your definition of `ebdb-add-to-list`
duplicates its argument, so

    (ebdb-add-to-list X (format FOO BAR))

becomes

    (when (format FOO BAR) (push (format FOO BAR) X))

Since `format` always returns a string, the `when` can be optimized away
so the above becomes:

    (progn (format FOO BAR) (push (format FOO BAR) X))

Beside the useless call to `format` there are other minor problems with
this definition of `ebdb-add-to-list`, such as the fact that its
arguments are not evaluated in the expected order (left to right), and
the fact that the second argument gets evaluated twice, so:

    (ebdb-add-to-list (progn (message "hello") 'x)
                      (progn (message "world") 3))

ends up adding 3 to x and emitting

    world
    world
    hello

instead of

    hello
    world

But the more serious problem is that `ebdb-add-to-list` is defined as
a function yet it can't work as a function because its first argument is
expected to be a "place" not a value.
So you have to define it as a macro rather than a (define-inline) function.

Also, personally I'd recommend you rename the to use a word like "push"
(and to switch its args) rather than `add-to-list`, so the reader can
guess that it takes an place, like `(cl-)push(new)`, rather than
a symbol, like `add-to-list`.


        Stefan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ebdb.patch --]
[-- Type: text/x-diff, Size: 9270 bytes --]

diff --git a/ebdb-complete.el b/ebdb-complete.el
index 444af05c30..220de71602 100644
--- a/ebdb-complete.el
+++ b/ebdb-complete.el
@@ -81,7 +81,7 @@
 
 (define-obsolete-function-alias
   'ebdb-complete-quit-window
-  'ebdb-complete-bury-ebdb-buffer "1.0.0")
+  #'ebdb-complete-bury-ebdb-buffer "1.0.0")
 
 (defun ebdb-complete-bury-ebdb-buffer ()
   "Hide EBDB buffer and switch to message buffer.
@@ -107,7 +107,7 @@ Before switch, this command will do some clean jobs."
 
 (define-obsolete-function-alias
   'ebdb-complete-push-mail-and-quit-window
-  'ebdb-complete-push-mail-and-bury-ebdb-buffer "1.0.0")
+  #'ebdb-complete-push-mail-and-bury-ebdb-buffer "1.0.0")
 
 (defun ebdb-complete-push-mail-and-bury-ebdb-buffer ()
   "Push email-address to Message window and hide EBDB buffer."
@@ -201,18 +201,18 @@ when in message body, this command will indent regular text."
 
 (defun ebdb-complete-keybinding-setup ()
   "Setup ebdb-complete Keybindings."
-  (define-key ebdb-mode-map "q" 'ebdb-complete-bury-ebdb-buffer)
-  (define-key ebdb-mode-map "\C-c\C-c" 'ebdb-complete-push-mail)
-  (define-key ebdb-mode-map (kbd "RET") 'ebdb-complete-push-mail-and-bury-ebdb-buffer))
+  (define-key ebdb-mode-map "q" #'ebdb-complete-bury-ebdb-buffer)
+  (define-key ebdb-mode-map "\C-c\C-c" #'ebdb-complete-push-mail)
+  (define-key ebdb-mode-map (kbd "RET") #'ebdb-complete-push-mail-and-bury-ebdb-buffer))
 
 ;;;###autoload
 (defun ebdb-complete-enable ()
   "Enable ebdb-complete, it will rebind TAB key in `message-mode-map'."
   (interactive)
   (require 'message)
-  (add-hook 'ebdb-mode-hook 'ebdb-complete-keybinding-setup)
-  (define-key message-mode-map "\t" 'ebdb-complete-message-tab)
-  (define-key mail-mode-map "\t" 'ebdb-complete-message-tab)
+  (add-hook 'ebdb-mode-hook #'ebdb-complete-keybinding-setup)
+  (define-key message-mode-map "\t" #'ebdb-complete-message-tab)
+  (define-key mail-mode-map "\t" #'ebdb-complete-message-tab)
   (message "ebdb-complete: Override EBDB keybindings: `q', `C-c C-c' and `RET'"))
 
 (provide 'ebdb-complete)
diff --git a/ebdb-i18n.el b/ebdb-i18n.el
index 49175d264e..5f24f3d7bf 100644
--- a/ebdb-i18n.el
+++ b/ebdb-i18n.el
@@ -51,8 +51,7 @@
 (defcustom ebdb-i18n-ignorable-scripts '(latin)
   "A list of script types that should be considered \"standard\".
 That is, no special handling will be done for them."
-  :type '(repeat :tag "Script type" symbol)
-  :group 'ebdb-i18n)
+  :type '(repeat :tag "Script type" symbol))
 
 (defcustom ebdb-i18ngroup-countries-pref-scripts nil
   "An alist equivalent to `ebdb-i18n-countries', but in alternate scripts.
@@ -66,8 +65,7 @@ Any country name listed here will be offered along with the
 English version for completion, and will be preferred over the
 English version for display."
 
-  :type '(repeat (cons string string))
-  :group 'ebdb-i18n)
+  :type '(repeat (cons string string)))
 
 ;; defvars come first to pacify compiler.
 
diff --git a/ebdb-message.el b/ebdb-message.el
index a32ae4e63c..7073dea3c0 100644
--- a/ebdb-message.el
+++ b/ebdb-message.el
@@ -64,7 +64,6 @@ configurations.
 
 Note that this should be a different symbol from that used in
 Gnus's article-reading config."
-  :group 'ebdb-mua-message
   :type '(choice (const nil)
 		 (symbol :tag "Window config name")))
 
@@ -167,7 +166,7 @@ Also fires when postponing a draft."
 		   #'ebdb-message-complete-mail-cleanup
 		   nil t)))
     ('nil nil)
-    (_ (define-key mail-mode-map "\M-\t" 'ebdb-complete-mail)))
+    (_ (define-key mail-mode-map "\M-\t" #'ebdb-complete-mail)))
 
   (when ebdb-message-window-configuration
     (add-to-list 'gnus-window-to-buffer
diff --git a/ebdb-mhe.el b/ebdb-mhe.el
index 57268fb718..9bc9c5450b 100644
--- a/ebdb-mhe.el
+++ b/ebdb-mhe.el
@@ -28,7 +28,6 @@
 (require 'mh-e)
 (if (fboundp 'mh-version)
     (require 'mh-comp))              ; For mh-e 4.x
-(require 'advice)
 
 (defgroup ebdb-mua-mhe nil
   "EBDB customizations for mhe."
@@ -144,16 +143,16 @@ Returns the empty string if HEADER is not in the message."
     (ebdb-load))
   (define-key mh-folder-mode-map ";" ebdb-mua-keymap)
   (when ebdb-complete-mail
-    (define-key mh-letter-mode-map "\M-;" 'ebdb-complete-mail)
-    (define-key mh-letter-mode-map "\e\t" 'ebdb-complete-mail)))
+    (define-key mh-letter-mode-map "\M-;" #'ebdb-complete-mail)
+    (define-key mh-letter-mode-map "\e\t" #'ebdb-complete-mail)))
 
 ;;;###autoload
 (defun ebdb-mhe-auto-update ()
   (ebdb-mua-auto-update ebdb-mhe-auto-update-p))
 
-(add-hook 'mh-show-hook 'ebdb-mhe-auto-update)
+(add-hook 'mh-show-hook #'ebdb-mhe-auto-update)
 
-(add-hook 'mh-folder-mode-hook 'ebdb-insinuate-mh)
+(add-hook 'mh-folder-mode-hook #'ebdb-insinuate-mh)
 
 (provide 'ebdb-mhe)
 ;;; ebdb-mhe.el ends here
diff --git a/ebdb-rmail.el b/ebdb-rmail.el
index b86d12cb15..4a4ed59675 100644
--- a/ebdb-rmail.el
+++ b/ebdb-rmail.el
@@ -90,9 +90,9 @@ the value of `ebdb-default-window-size'."
 (defun ebdb-rmail-auto-update ()
   (ebdb-mua-auto-update ebdb-rmail-auto-update-p))
 
-(add-hook 'rmail-mode-hook 'ebdb-insinuate-rmail)
+(add-hook 'rmail-mode-hook #'ebdb-insinuate-rmail)
 
-(add-hook 'rmail-show-message-hook 'ebdb-rmail-auto-update)
+(add-hook 'rmail-show-message-hook #'ebdb-rmail-auto-update)
 
 (provide 'ebdb-rmail)
 ;;; ebdb-rmail.el ends here
diff --git a/ebdb-vm.el b/ebdb-vm.el
index cdac018046..d7cbfd0cf2 100644
--- a/ebdb-vm.el
+++ b/ebdb-vm.el
@@ -118,26 +118,22 @@ the value of `ebdb-default-window-size'."
 (defcustom ebdb/vm-auto-folder-headers '("From:" "To:" "CC:")
   "The headers used by `ebdb/vm-auto-folder'.
 The order in this list is the order how matching will be performed."
-  :group 'ebdb-mua-vm
   :type '(repeat (string :tag "header name")))
 
 ;;;###autoload
 (defcustom ebdb/vm-auto-folder-field "vm-folder"
   "The xfield which `ebdb/vm-auto-folder' searches for."
-  :group 'ebdb-mua-vm
   :type 'symbol)
 
 ;;;###autoload
 (defcustom ebdb/vm-virtual-folder-field "vm-virtual"
   "The xfield which `ebdb/vm-virtual-folder' searches for."
-  :group 'ebdb-mua-vm
   :type 'symbol)
 
 ;;;###autoload
 (defcustom ebdb/vm-virtual-real-folders nil
   "Real folders used for defining virtual folders.
 If nil use `vm-primary-inbox'."
-  :group 'ebdb-mua-vm
   :type '(choice (const :tag "Use vm-primary-inbox" nil)
                  (repeat (string :tag "Real folder"))))
 
@@ -255,7 +251,6 @@ Its elements may be strings used both as the field value to check for
 and as the label to apply to the message.
 If an element is a cons pair (VALUE . LABEL), VALUE is the field value
 to search for and LABEL is the label to apply."
-  :group 'ebdb-mua-vm
   :type '(repeat string))
 
 (defcustom ebdb/vm-auto-add-label-field 'ebdb-mail-alias-field
@@ -263,7 +258,6 @@ to search for and LABEL is the label to apply."
 This is either a single EBDB field or a list of fields that
 `ebdb/vm-auto-add-label' uses to check for labels to apply to a message.
 Defaults to `ebdb-mail-alias-field' which defaults to `mail-alias'."
-  :group 'ebdb-mua-vm
   :type '(choice symbol (repeat symbol)))
 
 (defun ebdb/vm-auto-add-label (record)
@@ -310,7 +304,7 @@ from different senders."
                             ebdb/vm-auto-add-label-list))))
         (if labels
             (vm-add-message-labels
-             (mapconcat 'identity labels " ") 1)))))
+             (mapconcat #'identity labels " ") 1)))))
 
 \f
 
@@ -372,11 +366,11 @@ from different senders."
   (unless ebdb-db-list
     (ebdb-load))
   (define-key vm-mode-map ";" ebdb-mua-keymap)
-  (define-key vm-mode-map "/" 'ebdb)
+  (define-key vm-mode-map "/" #'ebdb)
   ;; `mail-mode-map' is the parent of `vm-mail-mode-map'.
   ;; So the following is also done by `ebdb-insinuate-mail'.
   (if (and ebdb-complete-mail (boundp 'vm-mail-mode-map))
-      (define-key vm-mail-mode-map "\M-\t" 'ebdb-complete-mail))
+      (define-key vm-mail-mode-map "\M-\t" #'ebdb-complete-mail))
 
   ;; Set up user field for use in `vm-summary-format'
   ;; (1) Big solution: use whole name
diff --git a/ebdb.el b/ebdb.el
index e4861c2756..d73f82752a 100644
--- a/ebdb.el
+++ b/ebdb.el
@@ -916,17 +916,19 @@ You really should not disable debugging.  But it will speed things up."
 ;; directly.  But presumably we could replace all the object-*
 ;; functions with the ebdb-* inlines.
 
-(define-inline ebdb-add-to-list (list-var element)
+(defmacro ebdb-add-to-list (list-var element) ;Rename to *push* and swap args?
   "Add ELEMENT to the value of LIST-VAR if it isn't there yet and non-nil.
 The test for presence of ELEMENT is done with `equal'."
-  (inline-quote (when ,element (cl-pushnew ,element ,list-var :test #'equal))))
+  (macroexp-let2 nil element element
+    `(when ,element (cl-pushnew ,element ,list-var :test #'equal))))
 
-(define-inline ebdb-remove-from-list (list-var element)
+(defmacro ebdb-remove-from-list (list-var element) ;Rename and swap args?
   "Remove ELEMENT from LIST-VAR, if present.
 Test for presence is done with `equal'."
-  (inline-quote (when (and ,element ,list-var)
-		  (setf ,list-var
-			(delete ,element ,list-var)))))
+  (macroexp-let2 nil element element
+    `(when ,element
+       ,(gv-letplace (getter setter) list-var
+	  (funcall setter `(delete ,element ,getter))))))
 
 ;;; Struct and object definitions.