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

llama: string_split fix #10022

Merged
merged 2 commits into from
Oct 25, 2024
Merged

llama: string_split fix #10022

merged 2 commits into from
Oct 25, 2024

Conversation

Xarbirus
Copy link
Contributor

The change fixes parsing of strings that contain spaces.
The non-template version works correctly, but in the template version operator>> stops reading at a space (which may be contained in the model path, for example).

@ngxson
Copy link
Collaborator

ngxson commented Oct 24, 2024

I'm wondering, can we also explicit disable the template<class T> version with std::enable_if? Something like this:

template<typename T>
static typename std::enable_if<std::negation<std::is_same<T, std::string>::value>, bool>::type

This is just to make sure that if someone accidentally call string_split without specifying <std::string>, then it never use the template version

@slaren
Copy link
Collaborator

slaren commented Oct 24, 2024

The template is already specialized for std::string, so I don't think that's necessary. Personally, I would very much prefer to avoid unreadable SFINAE nonsense, a static_assert would do the same if absolutely necessary.

…orrect template specialization is used for std::string
@Xarbirus
Copy link
Contributor Author

Now we will not be able to call string_split without specifying std::string, because we will get an error like

error: no matching function for call to 'string_split'
note: candidate template ignored: couldn't infer template argument 'T'

But I added a static_assert in case the specialized version is accidentally removed.

@slaren slaren merged commit d80fb71 into ggerganov:master Oct 25, 2024
53 checks passed
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Nov 15, 2024
* llama: Refactor string_split to use template specialization,  fixes parsing strings with spaces

* llama: Add static_assert in the string_split template to ensure the correct template specialization is used for std::string
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Nov 18, 2024
* llama: Refactor string_split to use template specialization,  fixes parsing strings with spaces

* llama: Add static_assert in the string_split template to ensure the correct template specialization is used for std::string
@Xarbirus Xarbirus deleted the string_split_fix branch January 20, 2025 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants