replace-using uses dotimes incorrectly

[Yehouda]

https://github.com/vindarel/cl-str/blob/b1c83802a323ebd843b82b30c8473c84cb09cea3/str.lisp#L359

replace-using tries to pick two elements in each itretaion by incrementing the count (i) in the body of dotimes, but that doesn't work because the counter i is bound again in each itretaion to each integer from 0 to the value of the count-form. So as it is, replace-using does replace-all withe first and second element, and then withe secnd and third (rather than third and fourth as it is documented). http://www.lispworks.com/documentation/lw51/CLHS/Body/m_dotime.htm

That causes the test that does (REPLACE-USING (LIST "a" "o" "A") "faaAA") to fail, because after replacing "a" by "o", it replaces "o" by "A".

I fixed it n my copy by replacing the dotimes form with this form:

(loop for (from to) on plist by 'cddr do
          (when to  ; this check is needed because plist may be odd length. Maybe should error on that. 
            (setq s (str:replace-all from to s))))

[kilianmh]

Hi @Yehouda thank you for your first issue here!

Indeed, replace-using is not working as intended right now.

Here is a version that should replace as intended, and additionally errors when the replacement-list element count is odd, or when there are duplicated keys. What you think @vindarel?

(defun replace-using (plist s)
  (when (oddp (list-length plist))
    (error "The replacement-list ~A needs to have an even number of elements." plist))
  (let ((key-list nil)
        (duplicates nil))
    (loop for (key value) on plist by #'cddr do
      (push key key-list))
    (loop for list on key-list
          do (let ((item (car list)))
               (when (member item
                             (cdr list)
                             :test #'string=)
                 (pushnew item duplicates :test #'string=))))
    (when duplicates
      (error "The keys ~A occur more than once in the replacement-list."
             duplicates))
    (loop for (key value) on plist by #'cddr do
      (setf s (str:replace-all key value s)))
    s))

[vindarel]

Hello, strange, is this line not giving the same result for you?

(str:replace-using (list "a" "o" "A") "faaAA")
;; => "fooAA"

which is as advertised.

---

Any improvement is welcome +1, thanks.

As soon as we throw errors, we should create a condition for the STR package?

[Yehouda]

Without fixing it. (str:replace-using (list "a" "o" "A") "faaAA") retruns for me "fAAAA" (on LispWorks). That is because when i is 0 in the dotimes, it replaces the "o" by "a" to get "faaAA", and when i is 1 it replaces "a" by "A" to get "fAAAA".

If it doesn't do that for you, it suggests that i is never 1 for you, which may happen if dotimes doesn't rebind i again as it is supposed to do according to the standard. Which lisp is this? you can try this:

(dotimes (i 3) (princ (incf i)))

that should print "123." If dotimes doesn't rebind, it will print "13". You can also expand the dotimes form to see what it does.

About errors, adding conditions is not actually useful, because users will not bother to have handlers for such conditions. What you need is a very clear error message that tells the user exactly what is the error in their code. For example, I tend to add the function name to the error message, .e.g:

"REPLACE-USING: The replacement-list ~A needs to have an even number of elements."

Also would change the name of the variable in the lambda list to replacement-list, so what function-lambda-list returns matches the error message.

[vindarel]

(dotimes (i 3) (princ (incf i)))
13
NIL

on SBCL 2.0.1

and I can agree for the condition and especially for the error message.

thx.

[Yehouda]

That is a bug in SBCL.