-
Notifications
You must be signed in to change notification settings - Fork 109
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
Vkm add some missing functions (#330) #334
Vkm add some missing functions (#330) #334
Conversation
every replaced by more sequences version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few inline comments.
Also, as a general comment, please review the indentation and style to make sure it is consistent. I see 4 spaces in some functions, 1 in others. Let's use 2 spaces as we are using in other files 👍
src/list.lisp
Outdated
(defun union (list1 list2 &key key (test #'eq)) | ||
(cond ((and list1 list2) | ||
(let ((result (makeset list2 :test #'equal))) | ||
(dolist (it (makeset list1 :test #'equal)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you actually need makeset
for the dolist
? here? I don't think so. And it would probably be faster if you don't call it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
These are my cats on the keyboard ran :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About indentation - accepted. That's my fault.
In my standard uses lisp-body-indentation 4, but for JSCL PR I will use indent for 2 positions. Perhaps individual keywords will be with indented 1, sorry, each emacs has their own habits.
Thanks for review. |
added commentary added (vectorp) check with error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged. Thanks! One PR closer to CLOS 👍 😃
What
sort
(implementation only for lists, others in TODO) in list.lispset-difference
in list.lispunion
in list.lispremove-duplicates
in sequence.lispevery
in sequence.lisp replaced by more sequences versionReasons