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

add overload add(a: var string, b: openArray[char]) #15951

Merged

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Nov 13, 2020

2 goals:

  • it's currently hard to convert openArray[char] to a string (unless you do it manually, essentially inlining the code in this PR)
  • avoid allocations when possible if user already has a y openArray to begin with

future work

@timotheecour timotheecour force-pushed the pr_add_string_openArray_char branch from a96cf06 to 47773e9 Compare November 13, 2020 06:49
changelog.md Outdated Show resolved Hide resolved
changelog.md Outdated Show resolved Hide resolved
lib/system.nim Outdated Show resolved Hide resolved
lib/system.nim Outdated Show resolved Hide resolved
@metagn
Copy link
Collaborator

metagn commented Nov 14, 2020

While the intention here is good there are so many things in the stdlib that could benefit from using a more generic char sequence type (like in strutils) and openarray[char] is not the best candidate, lest we snowball into doing this everywhere. This change is however small and mirrors add(var seq[T], openarray[T]), so it's fine, except that that overload doesn't copy memory directly and uses move/sink instead

@juancarlospaco
Copy link
Collaborator

A snowball has to start somewhere... 😼

@timotheecour
Copy link
Member Author

openarray[char] is not the best candidate

string/seq/array implicitly converts to openArray[char], which is the main point; I'm not sure what your other "best candidate" is (apart from std/views see #14869 which has the advantage of being 1st class but doesn't implicitly convert from string/seq/array; only explicitly)

mirrors add(var seq[T], openarray[T]), so it's fine, except that that overload doesn't copy memory directly and uses move/sink instead

for openArray[char] (this PR) memcpy (or memmove if memory overlaps) is the correct thing to do; for the other existing overload with seq[T], we could/should do the same depending on T

@timotheecour timotheecour force-pushed the pr_add_string_openArray_char branch from a32d472 to 9104ef3 Compare February 19, 2021 09:52
@Araq
Copy link
Member

Araq commented Feb 24, 2021

Can we add this to the new strbasics instead?

@timotheecour timotheecour force-pushed the pr_add_string_openArray_char branch from 99fc861 to c1649ac Compare February 24, 2021 18:27
@timotheecour
Copy link
Member Author

timotheecour commented Feb 24, 2021

Can we add this to the new strbasics instead?

done
@Araq PTAL

I'll followup with a PR to address this:

xxx use nimCopyMem(x[n].addr, y[0].addr, y.len) after some refactoring

with some refactoring from compiler/strutils2

@timotheecour timotheecour force-pushed the pr_add_string_openArray_char branch from a73a474 to 69a3270 Compare February 25, 2021 22:22
@timotheecour timotheecour force-pushed the pr_add_string_openArray_char branch from d658a9d to d9245a1 Compare February 26, 2021 00:30
@Araq Araq added the merge_when_passes_CI mergeable once green label Mar 1, 2021
@timotheecour timotheecour merged commit 0cb02fb into nim-lang:devel Mar 1, 2021
@timotheecour timotheecour deleted the pr_add_string_openArray_char branch March 1, 2021 15:51
@timotheecour timotheecour added the TODO: followup needed remove tag once fixed or tracked elsewhere label Mar 1, 2021
ringabout pushed a commit to ringabout/Nim that referenced this pull request Mar 22, 2021
ringabout added a commit to ringabout/Nim that referenced this pull request Mar 23, 2021
@ringabout ringabout mentioned this pull request Mar 24, 2021
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
@metagn metagn removed the TODO: followup needed remove tag once fixed or tracked elsewhere label Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_when_passes_CI mergeable once green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants