Re: Byte-compilation of custom themes

[Basil L. Contovounesios]

Eli Zaretskii <address@hidden> writes:

>> From: "Basil L. Contovounesios" <address@hidden>
>> Date: Thu, 10 May 2018 03:49:35 +0100
>> Cc: address@hidden
>> 
>> diff --git a/lisp/custom.el b/lisp/custom.el
>> index 1fed5dce53..b286f49ff9 100644
>> --- a/lisp/custom.el
>> +++ b/lisp/custom.el
>> @@ -1299,17 +1299,15 @@ custom-available-themes
>>  loaded, and no effort is made to check that the files contain
>>  valid Custom themes.  For a list of loaded themes, check the
>>  variable `custom-known-themes'."
>> -  (let (sym themes)
>> +  (let ((suffix "-theme\\.el\\'")
>> +        themes)
>>      (dolist (dir (custom-theme--load-path))
>> -      (when (file-directory-p dir)
>> -    (dolist (file (file-expand-wildcards
>> -                   (expand-file-name "*-theme.el" dir) t))
>> -      (setq file (file-name-nondirectory file))
>> -      (and (string-match "\\`\\(.+\\)-theme.el\\'" file)
>> -           (setq sym (intern (match-string 1 file)))
>> -           (custom-theme-name-valid-p sym)
>> -           (push sym themes)))))
>> -    (nreverse (delete-dups themes))))
>> +      (dolist (file (directory-files dir nil suffix))
>
> The original code carefully verified that the members in
> custom-theme--load-path are directories, whereas your proposal calls
> directory-files on each member unconditionally, which will barf if a
> file is not a directory.

The function custom-theme--load-path already incorporates the
file-directory-p check, so it is technically redundant here.
Would you rather it be kept regardless?

>> -    (define-key map "\C-x\C-s" 'custom-theme-write)
>> -    (define-key map "q" 'Custom-buffer-done)
>> -    (define-key map "n" 'widget-forward)
>> -    (define-key map "p" 'widget-backward)
>> +    (define-key map "\C-x\C-s" #'custom-theme-write)
>> +    (define-key map "q" #'Custom-buffer-done)
>> +    (define-key map "n" #'widget-forward)
>> +    (define-key map "p" #'widget-backward)
>
> Really?  Are we going to switch to #'foo even in key bindings?

Though I personally prefer to consistently #'-quote function symbols in
my own code, both for the extra byte-compiler check and narrower
in-buffer completion, I have no strong opinion here; I was simply making
the change in a sweeping fashion, in line with what I had perceived as a
welcome style.  Out of curiosity, though, what makes key bindings
special w.r.t. quoting?

>> diff --git a/lisp/custom.el b/lisp/custom.el
>> index b8004cfd74..ae917c0292 100644
>> --- a/lisp/custom.el
>> +++ b/lisp/custom.el
>> @@ -633,14 +633,10 @@ custom-load-symbol
>>      (let ((custom-load-recursion t))
>>        ;; Load these files if not already done,
>>        ;; to make sure we know all the dependencies of SYMBOL.
>> -      (condition-case nil
>> -      (require 'cus-load)
>> -    (error nil))
>> -      (condition-case nil
>> -      (require 'cus-start)
>> -    (error nil))
>> +      (require 'cus-load nil t)
>> +      (require 'cus-start nil t)
>>        (dolist (load (get symbol 'custom-loads))
>> -    (cond ((symbolp load) (condition-case nil (require load) (error nil)))
>> +        (cond ((symbolp load) (require load nil t))
>>            ;; This is subsumed by the test below, but it's much faster.
>>            ((assoc load load-history))
>>            ;; This was just (assoc (locate-library load) load-history)
>> @@ -658,7 +654,7 @@ custom-load-symbol
>>            ;; We are still loading it when we call this,
>>            ;; and it is not in load-history yet.
>>            ((equal load "cus-edit"))
>> -          (t (condition-case nil (load load) (error nil))))))))
>> +              (t (load load t)))))))
>
> Why are we dropping the safety nets here?

Because it hadn't occurred to silly us that we might want to
additionally protect against evaluation errors.

I reattach the patches, updated for the last two points of feedback and
to remove the duplicate 'Custom Themes' heading.

Thanks,

-- 
Basil

0001-Improve-loading-of-byte-compiled-custom-themes.patch
Description: Text Data

0002-Disable-no-byte-compile-in-built-in-themes.patch
Description: Text Data

0003-Fix-custom-available-themes-file-expansion.patch
Description: Text Data

0004-lisp-custom.el-Use-lexical-binding.patch
Description: Text Data

0005-lisp-cus-theme.el-Use-lexical-binding.patch
Description: Text Data

0006-Minor-custom.el-simplifications.patch
Description: Text Data

0007-Minor-cus-theme.el-simplifications.patch
Description: Text Data

0008-Minor-subr-x-tweaks.patch
Description: Text Data