Skip to content

Commit

Permalink
fixes #9306 properly, fixes #9306
Browse files Browse the repository at this point in the history
  • Loading branch information
Araq committed Oct 14, 2018
1 parent b62328c commit 8955470
Showing 1 changed file with 12 additions and 2 deletions.
14 changes: 12 additions & 2 deletions lib/impure/re.nim
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,8 @@ iterator findAll*(buf: cstring, pattern: Regex, start = 0, bufSize: int): string
proc findAll*(s: string, pattern: Regex, start = 0): seq[string] {.inline.} =
## returns all matching `substrings` of ``s`` that match ``pattern``.
## If it does not match, @[] is returned.
accumulateResult(findAll(s, pattern, start))
result = @[]
for x in findAll(s, pattern, start): result.add x

when not defined(nimhygiene):
{.pragma: inject.}
Expand Down Expand Up @@ -433,6 +434,7 @@ proc replace*(s: string, sub: Regex, by = ""): string =
if match.first < 0: break
add(result, substr(s, prev, match.first-1))
add(result, by)
if match.last + 1 == prev: break
prev = match.last + 1
add(result, substr(s, prev))

Expand All @@ -458,6 +460,7 @@ proc replacef*(s: string, sub: Regex, by: string): string =
if match.first < 0: break
add(result, substr(s, prev, match.first-1))
addf(result, by, caps)
if match.last + 1 == prev: break
prev = match.last + 1
add(result, substr(s, prev))

Expand Down Expand Up @@ -543,7 +546,8 @@ proc split*(s: string, sep: Regex, maxsplit = -1): seq[string] {.inline.} =
## Splits the string ``s`` into a seq of substrings.
##
## The portion matched by ``sep`` is not returned.
accumulateResult(split(s, sep))
result = @[]
for x in split(s, sep): result.add x

proc escapeRe*(s: string): string =
## escapes ``s`` so that it is matched verbatim when used as a regular
Expand Down Expand Up @@ -674,3 +678,9 @@ when isMainModule:
accum.add($x)
doAssert(accum == @["a","b","c"])

block:
# bug #9306
doAssert replace("bar", re"^", "foo") == "foobar"
doAssert replace("foo", re"", "-") == "-foo"

This comment has been minimized.

Copy link
@kaushalmodi

kaushalmodi Oct 14, 2018

Contributor

This second test is not correct. The replacement should match with "-f-o-o-". Looks like it's treating "" re the same as "^"?

This comment has been minimized.

Copy link
@Araq

Araq Oct 14, 2018

Author Member

As long as it doesn't run into endless loops, it's fine, replacing empty matches is madness.

This comment has been minimized.

Copy link
@kaushalmodi

kaushalmodi Oct 14, 2018

Contributor

But that behavior is not pcre compatible or any regex compatible for that matter.

Why should we have wrong logic just to avoid loops?

This comment has been minimized.

Copy link
@Araq

Araq Oct 14, 2018

Author Member

There is no wrong logic here, we don't claim to be bug-compatible with other regex engines.

This comment has been minimized.

Copy link
@timotheecour

timotheecour Oct 29, 2018

Member

=> discussed here: #9437

doAssert replace("foo", re"$", "bar") == "foobar"

0 comments on commit 8955470

Please sign in to comment.