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 (counsel-{file,dired}-jump): use temporary buffers as compared to split-string #2120

Closed
wants to merge 4 commits into from

Conversation

@phikal
Copy link
Contributor

@phikal phikal commented Jul 3, 2019

This basically just replaces the higher level "split-string" with a more low level loop to improve performance when indexing deep directory trees. On my system, I get a 10%-20% performance boost, together with up to 20% reduction in garbage collection.

Another note would be that unless it's against some rule, it might not be bad to convert counsel-file-jump-args and counsel-dired-jump-args directly into lists, rather than strings that have to be split again.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jul 3, 2019

Thanks. Looks good; perhaps would be nice to factor out the common functionality into a separate defun.

This change is over 15 lines. I don't see your email on the copyright.list. Do you have an Emacs CA?

@phikal
Copy link
Contributor Author

@phikal phikal commented Jul 3, 2019

Thanks. Looks good; perhaps would be nice to factor out the common functionality into a separate defun.

Yeah, that can be done. But I guess such a general refactoring would merit it's own pull request.

This change is over 15 lines. I don't see your email on the copyright.list. Do you have an Emacs CA?

No, could you give me a link or a hint at what I would have to do? Never mind, found the link in the README.

abo-abo
abo-abo approved these changes Jul 4, 2019
@phikal
Copy link
Contributor Author

@phikal phikal commented Jul 23, 2019

Should be approved.

@abo-abo abo-abo closed this in ec3e062 Jul 24, 2019
abo-abo added a commit that referenced this issue Jul 24, 2019
* counsel.el (counsel-dired-jump-args): Now a list. Removing the
single quotes around '.git' breaks user's config anyway.
A user warning about the new format is added.

(counsel--find-return-list): Simplify. Use `process-file'.
TODO: merge this with `counsel--command'.

Re #2120
abo-abo added a commit that referenced this issue Jul 24, 2019
* counsel.el (counsel--command): Now forwards to `counsel--call'.
(counsel--call): Remove obsolete alias. Introduce as a generalization of `counsel--command'

Re #2120
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jul 24, 2019

Thanks, please review the follow-up commits.

@phikal
Copy link
Contributor Author

@phikal phikal commented Jul 25, 2019

Can't complain.

@phikal
Copy link
Contributor Author

@phikal phikal commented Nov 11, 2019

I was just thinking about this again, and wondered what's the point of reversing the list again in counsel--find-return-list? I know nreverse is relatively cheep, but it still seems like needless overhead, for an operation that is already kind-of ordered, but at the same time is more of a set than a sequence.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Nov 11, 2019

The point is to have the list in the alphabetical order. Even the filtered list will still be in that order.

astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
counsel (counsel-dired-jump): Use temp buffer instead of split-string

counsel (counsel-{file,dired}-jump): Avoid nbutlast

counsel (counsel-{file,dired}-jump): Factored out common code

Fixes abo-abo#2120
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
* counsel.el (counsel-dired-jump-args): Now a list. Removing the
single quotes around '.git' breaks user's config anyway.
A user warning about the new format is added.

(counsel--find-return-list): Simplify. Use `process-file'.
TODO: merge this with `counsel--command'.

Re abo-abo#2120
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
* counsel.el (counsel--command): Now forwards to `counsel--call'.
(counsel--call): Remove obsolete alias. Introduce as a generalization of `counsel--command'

Re abo-abo#2120
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants