Clojure

clojure.string/replace-first returns nil with replacement fn when regex doesn't match

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Backlog
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Vetted

Description

Originally reported by Takahiro Hozumi

With a function as the "replacement" argument, clojure.string/replace-first returns nil if there is no match, instead of returning the original string unchanged.

user=> (use 'clojure.string)
nil
user=> (replace-first "abcdef" "ghi" "jkl")
"abcdef"
user=> (replace-first "abcdef" #"ghi" "jkl")
"abcdef"
user=> (replace-first "abcdef" #"ghi" (fn [a] "jkl"))
nil

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Federico Brubacher added a comment -

This is my patch for this issue. I have a CA signed. Any suggestions and i can try again. Federico

Show
Federico Brubacher added a comment - This is my patch for this issue. I have a CA signed. Any suggestions and i can try again. Federico
Hide
Permalink
Chris Perkins added a comment -

Same fix as Frederico's patch, but also removes unnecessary allocation of a StringBuffer when there is no match, for both replace and replace first.

Show
Chris Perkins added a comment - Same fix as Frederico's patch, but also removes unnecessary allocation of a StringBuffer when there is no match, for both replace and replace first.
Hide
Permalink
Chris Perkins added a comment -

Same again, but with docstring typo fixes too.

Show
Chris Perkins added a comment - Same again, but with docstring typo fixes too.
Hide
Permalink
Steve Miner added a comment -

You should use StringBuilder instead of StringBuffer.

http://stackoverflow.com/questions/355089/stringbuilder-and-stringbuffer-in-java

Show
Steve Miner added a comment - You should use StringBuilder instead of StringBuffer. http://stackoverflow.com/questions/355089/stringbuilder-and-stringbuffer-in-java
Hide
Permalink
Chris Perkins added a comment -

Yup, that's what I thought, but it turns out it doesn't work because java's Matcher/appendReplacement requires a StringBuffer, unfortunately.

Show
Chris Perkins added a comment - Yup, that's what I thought, but it turns out it doesn't work because java's Matcher/appendReplacement requires a StringBuffer, unfortunately.
Hide
Permalink
Andy Fingerhut added a comment - - edited

I've tested Chris's clojure-string-with-no-match-returns-original-2.patch against latest github Clojure as of this morning, and the tests pass. Cool that he found a straightforward performance improvement for the no match case. The code appears correct. I also verified that for the reason Chris mentions, StringBuilder will not work as a replacement for StringBuffer. At least there will never be any contention on the locks implementing the StringBuffer synchronization the way they are used here.

Show
Andy Fingerhut added a comment - - edited I've tested Chris's clojure-string-with-no-match-returns-original-2.patch against latest github Clojure as of this morning, and the tests pass. Cool that he found a straightforward performance improvement for the no match case. The code appears correct. I also verified that for the reason Chris mentions, StringBuilder will not work as a replacement for StringBuffer. At least there will never be any contention on the locks implementing the StringBuffer synchronization the way they are used here.
Hide
Permalink
Stuart Sierra added a comment -

Vetted & moved to Approved Backlog.

Show
Stuart Sierra added a comment - Vetted & moved to Approved Backlog.
Hide
Permalink
Andy Fingerhut added a comment -

CLJ-870 has an attached proposed combined patch for this issue and that one.

Show
Andy Fingerhut added a comment - CLJ-870 has an attached proposed combined patch for this issue and that one.
Hide
Permalink
Andy Fingerhut added a comment -

This issue should now be corrected with the patch, attached to CLJ-870, that was applied on Aug 18, 2012.

Show
Andy Fingerhut added a comment - This issue should now be corrected with the patch, attached to CLJ-870, that was applied on Aug 18, 2012.
Hide
Permalink
Stuart Sierra added a comment -

CLJ-753 patch applied for CLJ-870 addresses this one, too, applied Aug 18, 2012

Show
Stuart Sierra added a comment - CLJ-753 patch applied for CLJ-870 addresses this one, too, applied Aug 18, 2012

People

Vote (2)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.