Added optional parameter max to replace functions#18194
Added optional parameter max to replace functions#18194kintrix007 wants to merge 17 commits intonim-lang:develfrom
max to replace functions#18194Conversation
if 'maxOccurrences' is -1, which is the default, it replaces all occurrances. Can be optionally specified to only replace first 'maxOccurrences' ones. I couldn't think of a nice, and efficient way to implement this in multiReplace, to keep it being one iteration... I could implement it using a table to track how many times it replaced given strings, or I could just implement a 'multiReplacesFirsts' function
Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
kintrix007
changed the title
max to replace functions
lib/pure/strutils.nim
Outdated
| # copy the rest: | ||
| result.add s.substr(i) | ||
|
|
||
| # Maybe implement something like this for all replace functions...? |
There was a problem hiding this comment.
@kintrix007 Why do we have this commented block here? Should this instead be a separate issue or RFC?
There was a problem hiding this comment.
We do not.
I left it in case someone had something to say about it.
Will definitely remove it if the PR gets accepted.
But now that you mention it, I should have probably just said that as a comment here...
There was a problem hiding this comment.
Will definitely remove it if the PR gets accepted.
If the PR gets accepted, someone will just merge it. So it's better to keep the PR in the state you want to see it merged.
There was a problem hiding this comment.
also, the implementation is not good (no early break), so if you want to keep a comment just add:
# consider adding: func replace*(s: string, subs: set[char], by: char, max = -1): string (whether it's a good idea or not, is unclear)
lib/pure/strutils.nim
Outdated
| let j = find(a, s, sub, i) | ||
| if j == -1 or occLeft == 0: break | ||
| if occLeft > 0: dec occLeft | ||
| result.add s.substr(i, j - 1) |
There was a problem hiding this comment.
pre-existing but this creates un-necessary allocations.
IMO we need a 0-allocation [1] addReplace in strbasics and then strutils.replace can reuse it.
not a blocker for this PR though as this refactoring can be done later.
proc addReplace*(result: var string, input: openArray[char], pattern: string, by: string, max = -1): int
## returns the number of `pattern` instances found in `input` and replaced by `by`,
## the result being appended to `result`(0-alloc because user can reuse the result buffer)
There was a problem hiding this comment.
We should use a TR macro to optimize this, the pattern is common and while we can avoid it, many of our users don't know about it.
There was a problem hiding this comment.
that won't make replace 0-alloc though, hence the need for strbasics.addReplace in future work.
Note that for that particular line, all that's needed is replace substr by toOpenArray:
result.add s.substr(i, j - 1)
=>
result.add s.toOpenArray(i, j - 1)
(with import std/strbasics on top), which is now possible since #15951
regarding TR here, maybe (but TR have their own issues)
tests fail after importing std/strbasics, and using toOpenArray, the code compiles and doc still succeeds
|
@kintrix007 the diff shows unrelated changes; also, I recommend to embrace git rebase instead of git merge (easier to follow PR's) |
I have no idea how those happened, as those are under |
|
This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions. |
4 participants
if
maxis-1, which is the default, it replaces all occurrences.Can be optionally specified to only replace the first
maxamount.I couldn't think of a nice, and relatively efficient way to implement this to
multiReplace, while keeping it being one iteration... I could implement it using a table to track how many times it replaced given strings, or I could probably just implement amultiReplacesFirstsfunction.