* Re: [elpa] externals/ebdb f7b50402cf: Final round of compiler quieting
2023-10-24 2:40 ` [elpa] externals/ebdb f7b50402cf: Final round of compiler quieting Stefan Monnier
@ 2023-10-25 18:18 ` Eric Abrahamsen
2023-12-15 1:11 ` Eric Abrahamsen
1 sibling, 0 replies; 5+ messages in thread
From: Eric Abrahamsen @ 2023-10-25 18:18 UTC (permalink / raw)
To: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> 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`.
Thank you very much for the detailed explanation, and for the patch!
This is an area of Elisp I still don't have much feel for, and I don't
think would have come up with the right gv-/macroexp- invocations
myself.
I'll get this released in the next few days.
Thanks again,
Eric
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [elpa] externals/ebdb f7b50402cf: Final round of compiler quieting
2023-10-24 2:40 ` [elpa] externals/ebdb f7b50402cf: Final round of compiler quieting Stefan Monnier
2023-10-25 18:18 ` Eric Abrahamsen
@ 2023-12-15 1:11 ` Eric Abrahamsen
2023-12-15 2:36 ` Stefan Monnier
1 sibling, 1 reply; 5+ messages in thread
From: Eric Abrahamsen @ 2023-12-15 1:11 UTC (permalink / raw)
To: Stefan Monnier; +Cc: emacs-devel
On 10/23/23 22:40 PM, Stefan Monnier wrote:
>> 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
>
> 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))))))
I'm finally getting to push this code! I decided that ebdb-add-to-list
simply wasn't necessary -- there were hardly any cases where the element
test was meaningful, and otherwise this could just be `ebdb-pushnew'
that delegates to `cl-pushnew' with a :test. I'll keep
ebdb-remove-from-list, with your re-definition, but couldn't guess how
to rename it appropriately. `pop' isn't right, I suppose it could be
`ebdb-delete-from-list' since it uses delete under the hood -- otherwise
I don't see a clear convention in place, did you have something specific
in mind?
Thanks as always!
Eric
^ permalink raw reply [flat|nested] 5+ messages in thread