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

glob is called twice #2932

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

glob is called twice #2932

wants to merge 2 commits into from

Conversation

rtczza
Copy link

@rtczza rtczza commented Apr 8, 2024

In this context, matcher.glob() already returns a Cow<'a, str> type, so there is no need to call the glob method again. The glob method has been mistakenly called twice in a row here. The correct code should directly convert the Cow<'a, str> to a String type and then push it into Vec. The to_string() method can be used here (which can accept Cow<'a, str> and clone or borrow as needed) to complete the conversion.

The above is my own understanding, not necessarily correct, welcome to discuss.

@eth-p
Copy link
Collaborator

eth-p commented Apr 8, 2024

Thank you for your contribution!

From my understanding of the original code, matcher is a GlobMatcher from the globset crate, and the first glob() function returns a reference to the Glob struct which created the matcher. The second glob() function returns an &str to the original glob pattern, and into() calls to_owned() on that.

I have concerns that replacing it with matcher.glob().to_string() would be a tiny performance regression. Glob does not implement the ToString trait directly, which means that to_string() is provided by a blanket implementation for the Display trait. The blanket implementation has to go through Display::fmt, which has more overhead than calling to_owned() on &str.

I also noticed that this adds a new syntax as well as your changes to get_syntax_mapping_to_paths, so you might want to rebase it to remove that.

@rtczza
Copy link
Author

rtczza commented Apr 9, 2024

has been rebased and this submission has been removed.

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.

None yet

2 participants