Sharedp and negative indices #58

christophejunke · 2020-10-05T06:51:12Z

This pull request is related to the following issues:

There is probably still work to do, this is more a draft to talk about the changes

christophejunke · 2020-10-05T06:57:38Z

test/test-str.lisp

@@ -9,6 +9,16 @@
 (setf prove:*enable-colors* t)
 (plan nil)

+(subtest "Index"


christophejunke · 2020-10-05T06:58:02Z

test/test-str.lisp

+      '(0 0 0 1 2 3 4 0 1 2 3 4 5 5 5)
+      "index wraps and saturates"))
+
+(subtest "Slice"


Needs more tests

christophejunke · 2020-10-05T07:02:52Z

str.lisp

+     (with-indices (index) (1+ (length s))
+       (concat (slice 0 index s t)
+               (string string/char)
+               (slice index (length s) s t))))


use nil/t instead of (length s)

Here we need a warning!

btw, we could enrich the warning's message, to explain where it comes from, and what is this change.

looks doable. With a warning first, and maybe with a key argument to control the behavior? If someone speaks up we can be more conservative.

I would appreciate a keyword to control the behavior. And, I am afraid code of mine would break so I am tempted to say, let the new behavior be optional and activated with the keyword argument.

Could we speak a bit more in the issue?

Yes, I am thinking about adding some if-out-of-bounds keyword argument, we can talk more about it in the issue, if you have questions or preferences.

vindarel · 2020-10-06T22:04:39Z

README.md

@@ -271,11 +271,22 @@ Examples:
  (is "" (substring 2 1 "abcd") "start is bigger than end")
 ```

+#### slice `(start end s &optional sharedp)`
+
+Same as substring, except the result might share storage with s.


and more importantly, what's a negative end.

vindarel · 2020-10-06T22:07:27Z

str.lisp

@@ -137,27 +151,126 @@
 (defun version ()
  (print +version+))

+;; as a dedicated condition to help detect it in tests
+;; (up to 0.18.1 a negative index would mean zero)


but not in all functions right? I have to check below.

vindarel · 2020-10-06T22:12:12Z

str.lisp

+      (t
+       (locally (declare (type (and fixnum (not (eql 0))) length))
+         (when (< index 0)
+           (warn 'negative-index))


unnecessary to warn for a new function? (it doesn't harm though)

added a warnp keyword argument thay may eventually be removed later

vindarel · 2020-10-06T22:25:06Z

str.lisp

+
+(defun index (string/length index)
+  ""
+  (declare (optimize (speed 3))


I don't think it is necessary nor a good thing to hardcode this. Explain?

this is not necessary, might be better to remove declarations

vindarel · 2020-10-06T22:28:21Z

str.lisp

+               (if (< quotient 0) 0 length))))))))
+
+(defmacro with-indices ((&rest indices) string &body body)
+  "Ensure INDICES are computed according to INDEX w.r.t. STRING in BODY.


Where does INDEX come from?

Function INDEX, updating comment

vindarel · 2020-10-06T22:56:10Z

str.lisp

+     (with-indices (index) (1+ (length s))
+       (concat (slice 0 index s t)
+               (string string/char)
+               (slice index (length s) s t))))


looks doable. With a warning first, and maybe with a key argument to control the behavior? If someone speaks up we can be more conservative.

I would appreciate a keyword to control the behavior. And, I am afraid code of mine would break so I am tempted to say, let the new behavior be optional and activated with the keyword argument.

Could we speak a bit more in the issue?

vindarel · 2020-10-06T22:57:25Z

str.lisp

    (if omit-nulls
-        (remove-if (lambda (it) (empty? it)) res)
+        (delete-if #'empty? res)


No. delete-if is destructive, we want to create a new string.

vindarel · 2020-10-06T22:59:16Z

str.lisp

@@ -250,14 +356,14 @@ It uses `subseq' with differences:
    (let ((end (max (- len (length ellipsis))
                    0)))
      (setf s (concat
-               (subseq s 0 end)
+               (slice% 0 end s t)


Why, what does that solve, is it a breaking change?

vindarel · 2020-10-06T22:59:58Z

str.lisp

               ellipsis))))
  s)

 (defun words (s &key (limit 0))
  "Return list of words, which were delimited by white space. If the optional limit is 0 (the default), trailing empty strings are removed from the result list (see cl-ppcre)."
  (when s
-      (cl-ppcre:split "\\s+" (trim-left s) :limit limit)))
+    (cl-ppcre:split "\\s+" (trim-left s) :limit limit :sharedp *sharedp*)))


add the keyword argument.

vindarel · 2020-10-06T23:02:43Z

str.lisp

 (defun prefix-1 (item1 item2)
-  (subseq item1 0 (or (mismatch item1 item2) (length item1))))
+  (slice 0 (mismatch item1 item2) item1))


review to be continued.

vindarel · 2020-10-06T23:05:18Z

There are too may changes. Please split the PR.

Some little changes would be ok to merge.

And we must discuss insert a bit more.

thanks!

Allows functions to return displaced arrays when *SHAREDP* is T.

Maybe this should be turned into GitHub Issues instead

- Introduce INDEX to give a canonical index computation in strings - Introduce WITH-INDICES to help canonicalize input indices - Warns when indices are negative because the behaviour differs from previous versions. The warning is an explicit condition class named NEGATIVE-INDEX so that this case can be handled more easily by user code that has automatic tests in place. - Adjust test cases for INSERT

christophejunke force-pushed the sharedp-and-negative-indices branch from 515d93e to 7b79c3a Compare October 5, 2020 06:54

christophejunke commented Oct 5, 2020

View reviewed changes

vindarel requested changes Oct 6, 2020

View reviewed changes

christophejunke added 6 commits October 8, 2020 22:51

Introduce *SHAREDP* and SLICE

cbbb534

Allows functions to return displaced arrays when *SHAREDP* is T.

Patch introducing FIXME? notes

1541938

Maybe this should be turned into GitHub Issues instead

Make REPEAT accept strings and characters

2e11ccd

Use REPEAT and CONCAT to simplify existing code

0d32545

Minor fixes: delete-if on local sequence, unlambda

ebdcb47

christophejunke force-pushed the sharedp-and-negative-indices branch from 7b79c3a to 1aad505 Compare October 8, 2020 22:05

vindarel closed this Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sharedp and negative indices #58

Sharedp and negative indices #58

christophejunke commented Oct 5, 2020

christophejunke Oct 5, 2020

christophejunke Oct 5, 2020

christophejunke Oct 5, 2020

vindarel Oct 6, 2020

vindarel Oct 6, 2020

christophejunke Oct 8, 2020

vindarel Oct 6, 2020

vindarel Oct 6, 2020

vindarel Oct 6, 2020

christophejunke Oct 8, 2020

vindarel Oct 6, 2020

christophejunke Oct 8, 2020

vindarel Oct 6, 2020

christophejunke Oct 8, 2020

vindarel Oct 6, 2020

vindarel Oct 6, 2020

vindarel Oct 6, 2020

vindarel Oct 6, 2020

vindarel Oct 6, 2020

vindarel commented Oct 6, 2020

Sharedp and negative indices #58

Sharedp and negative indices #58

Conversation

christophejunke commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vindarel commented Oct 6, 2020