Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

counsel.el (counsel-colors--name-to-hex): protect against invalid input. #1854

Merged
merged 1 commit into from Dec 12, 2018

Conversation

mookid
Copy link
Contributor

@mookid mookid commented Dec 11, 2018

fix #1853

@mookid mookid force-pushed the safename-to-hex branch 2 times, most recently from 4e932a9 to 12bb41a Compare Dec 11, 2018
'dups (cdr cell))))
(list-colors-duplicates)))
(let* ((colors
(delete nil
Copy link
Collaborator

@basil-conto basil-conto Dec 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use delq here.

Copy link
Contributor Author

@mookid mookid Dec 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, in fact it is probably better not to allocate two lists and use mapcan instead.

edit: this is misleading. delete does not copy.

Copy link
Collaborator

@basil-conto basil-conto Dec 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not always clear when it's better to use (delq nil (mapcar ...)) vs mapcan. The thing about mapcan is it was only added in Emacs 25, so you have to use the wrapper cl-mapcan, and you still need to allocate a cons cell in the loop for each element you want to keep:

(cl-mapcan (lambda (cell)
             (let* ((name (car cell))
                    (dups (cdr cell))
                    (hex (counsel-colors--name-to-hex name)))
               (and hex (list (propertize name 'hex hex 'dups dups)))))
           (list-colors-duplicates))

Benchmarking the two approaches suggests delq+mapcar is a hair faster and smaller than cl-mapcan they're pretty evenly matched. I really don't mind either way, I'm just saying there are tradeoffs, so it basically comes down to personal preference.

Copy link
Contributor Author

@mookid mookid Dec 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for running the experimentation.

Note that contrary to what I said, delete is destructive and does not make a new list (but remove does).

Copy link
Collaborator

@basil-conto basil-conto Dec 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that contrary to what I said, delete is destructive and does not make a new list (but remove does).

Yup. The difference between delq and delete is that the former only accepts lists and uses eq for equality, thus making it the standard function for use in (delq nil (mapcar ...)). Using delete is less common and reserved for cases where equivalence under equal is needed.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Dec 12, 2018

Thanks.

@abo-abo abo-abo merged commit a1b30ea into abo-abo:master Dec 12, 2018
1 of 2 checks passed
@mookid mookid deleted the safename-to-hex branch Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants