string-set! examples in r5rs.html

string-set! examples in r5rs.html

[Bill Schottstaedt]

according to r5rs.html, these should signal an error, I believe:

guile> (string-set! (symbol->string 'immutable)
             0
             #\?)
guile> (define (g) "***")
guile> (string-set! (g) 0 #\?)
guile> (g)
"?**"

Re: string-set! examples in r5rs.html

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 470 bytes --]

Hi,

"Bill Schottstaedt" <bil@ccrma.Stanford.EDU> writes:

> according to r5rs.html, these should signal an error, I believe:
>
> guile> (string-set! (symbol->string 'immutable)
>              0
>              #\?)
> guile> (define (g) "***")
> guile> (string-set! (g) 0 #\?)
> guile> (g)
> "?**"

The attached patches against 1.8.x fix this.

Neil: OK to apply?

If yes, I would eventually take this opportunity to make
`make-read-only-string' public.

Thanks,
Ludo'.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: First patch --]
[-- Type: text/x-patch, Size: 3312 bytes --]

From ee5cc17a4887ede871c256a222f097c256353971 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Mon, 22 Sep 2008 22:51:36 +0200
Subject: [PATCH] Make `symbol->string' return a read-only string.

* libguile/strings.c (scm_i_symbol_substring): Return a read-only string
  since R5RS requires `symbol->string' to return a read-only string.
  Reported by Bill Schottstaedt <bil@ccrma.Stanford.EDU>.

* test-suite/tests/symbols.test: Add `define-module' clause.
  (exception:immutable-string): Adjust to current exception.
  ("symbol->string")["result is an immutable string"]: Use
  `pass-if-exception' instead of `expect-fail-exception'.

* NEWS: Update.
---
 NEWS                          |    1 +
 libguile/strings.c            |    2 +-
 test-suite/tests/symbols.test |   12 ++++++------
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/NEWS b/NEWS
index f4ca739..9f6e0ed 100644
--- a/NEWS
+++ b/NEWS
@@ -26,6 +26,7 @@ available: Guile is now always configured in "maintainer mode".
 
 * Bugs fixed
 
+** `symbol->string' now returns a read-only string, as per R5RS
 ** `guile-config link' now prints `-L$libdir' before `-lguile'
 ** Fix memory corruption involving GOOPS' `class-redefinition'
 ** Fix possible deadlock in `mutex-lock'
diff --git a/libguile/strings.c b/libguile/strings.c
index 4d387fb..b0b9506 100644
--- a/libguile/strings.c
+++ b/libguile/strings.c
@@ -491,7 +491,7 @@ scm_i_symbol_substring (SCM sym, size_t start, size_t end)
   scm_i_pthread_mutex_lock (&stringbuf_write_mutex);
   SET_STRINGBUF_SHARED (buf);
   scm_i_pthread_mutex_unlock (&stringbuf_write_mutex);
-  return scm_double_cell (STRING_TAG, SCM_UNPACK(buf),
+  return scm_double_cell (RO_STRING_TAG, SCM_UNPACK(buf),
 			  (scm_t_bits)start, (scm_t_bits) end - start);
 }
 
diff --git a/test-suite/tests/symbols.test b/test-suite/tests/symbols.test
index b57667f..3fe3402 100644
--- a/test-suite/tests/symbols.test
+++ b/test-suite/tests/symbols.test
@@ -1,6 +1,6 @@
 ;;;; symbols.test --- test suite for Guile's symbols    -*- scheme -*-
 ;;;;
-;;;; Copyright (C) 2001, 2006 Free Software Foundation, Inc.
+;;;; Copyright (C) 2001, 2006, 2008 Free Software Foundation, Inc.
 ;;;; 
 ;;;; This program is free software; you can redistribute it and/or modify
 ;;;; it under the terms of the GNU General Public License as published by
@@ -17,17 +17,17 @@
 ;;;; the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
 ;;;; Boston, MA 02110-1301 USA
 
-(use-modules (ice-9 documentation))
+(define-module (test-suite test-symbols)
+  #:use-module (test-suite lib)
+  #:use-module (ice-9 documentation))
 
 
 ;;;
 ;;; miscellaneous
 ;;;
 
-;; FIXME:  As soon as guile supports immutable strings, this has to be
-;; replaced with the appropriate error type and message.
 (define exception:immutable-string
-  (cons 'some-error-type "^trying to modify an immutable string"))
+  (cons 'misc-error "^string is read-only"))
 
 (define (documented? object)
   (not (not (object-documentation object))))
@@ -55,7 +55,7 @@
 
 (with-test-prefix "symbol->string"
 
-  (expect-fail-exception "result is an immutable string"
+  (pass-if-exception "result is an immutable string"
     exception:immutable-string
     (string-set! (symbol->string 'abc) 1 #\space)))
 
-- 
1.6.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Second one --]
[-- Type: text/x-patch, Size: 4310 bytes --]

From 3edbf7465422b976dc26f64d9646e7e371a7b971 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Mon, 22 Sep 2008 23:03:20 +0200
Subject: [PATCH] Make literal strings (i.e., returned by `read') read-only.

* libguile/read.c (scm_read_string): Use `scm_i_make_read_only_string ()' to
  return a read-only string, as mandated by R5RS.  Reported by Bill
  Schottstaedt <bil@ccrma.Stanford.EDU>.

* libguile/strings.c (scm_i_make_read_only_string): New function.

* libguile/strings.h (scm_i_make_read_only_string): New declaration.

* test-suite/tests/strings.test ("string-set!")["literal string"]: New test.

* NEWS: Update.
---
 NEWS                          |    1 +
 libguile/read.c               |    2 +-
 libguile/strings.c            |   11 +++++++++++
 libguile/strings.h            |    3 ++-
 test-suite/tests/strings.test |    8 ++++++--
 5 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/NEWS b/NEWS
index 9f6e0ed..da88315 100644
--- a/NEWS
+++ b/NEWS
@@ -27,6 +27,7 @@ available: Guile is now always configured in "maintainer mode".
 * Bugs fixed
 
 ** `symbol->string' now returns a read-only string, as per R5RS
+** `read' now returns read-only strings, as per R5RS
 ** `guile-config link' now prints `-L$libdir' before `-lguile'
 ** Fix memory corruption involving GOOPS' `class-redefinition'
 ** Fix possible deadlock in `mutex-lock'
diff --git a/libguile/read.c b/libguile/read.c
index ff50735..9600ecc 100644
--- a/libguile/read.c
+++ b/libguile/read.c
@@ -514,7 +514,7 @@ scm_read_string (int chr, SCM port)
   else
     str = (str == SCM_BOOL_F) ? scm_nullstr : str;
 
-  return str;
+  return scm_i_make_read_only_string (str);
 }
 #undef FUNC_NAME
 
diff --git a/libguile/strings.c b/libguile/strings.c
index b0b9506..220aac2 100644
--- a/libguile/strings.c
+++ b/libguile/strings.c
@@ -218,6 +218,17 @@ get_str_buf_start (SCM *str, SCM *buf, size_t *start)
 }
 
 SCM
+scm_i_make_read_only_string (SCM str)
+{
+  if (SCM_UNLIKELY (STRING_LENGTH (str)) == 0)
+    /* We want the empty string to be `eq?' with the read-only empty
+       string.  */
+    return str;
+
+  return scm_i_substring_read_only (str, 0, STRING_LENGTH (str));
+}
+
+SCM
 scm_i_substring (SCM str, size_t start, size_t end)
 {
   SCM buf;
diff --git a/libguile/strings.h b/libguile/strings.h
index dcc1f11..c895708 100644
--- a/libguile/strings.h
+++ b/libguile/strings.h
@@ -3,7 +3,7 @@
 #ifndef SCM_STRINGS_H
 #define SCM_STRINGS_H
 
-/* Copyright (C) 1995,1996,1997,1998,2000,2001, 2004, 2005, 2006 Free Software Foundation, Inc.
+/* Copyright (C) 1995,1996,1997,1998,2000,2001, 2004, 2005, 2006, 2008 Free Software Foundation, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -82,6 +82,7 @@ SCM_API SCM scm_make_string (SCM k, SCM chr);
 SCM_API SCM scm_string_length (SCM str);
 SCM_API SCM scm_string_ref (SCM str, SCM k);
 SCM_API SCM scm_string_set_x (SCM str, SCM k, SCM chr);
+SCM_API SCM scm_i_make_read_only_string (SCM str);
 SCM_API SCM scm_substring (SCM str, SCM start, SCM end);
 SCM_API SCM scm_substring_read_only (SCM str, SCM start, SCM end);
 SCM_API SCM scm_substring_shared (SCM str, SCM start, SCM end);
diff --git a/test-suite/tests/strings.test b/test-suite/tests/strings.test
index aa9196e..735258a 100644
--- a/test-suite/tests/strings.test
+++ b/test-suite/tests/strings.test
@@ -1,7 +1,7 @@
 ;;;; strings.test --- test suite for Guile's string functions    -*- scheme -*-
 ;;;; Jim Blandy <jimb@red-bean.com> --- August 1999
 ;;;;
-;;;; Copyright (C) 1999, 2001, 2004, 2005, 2006 Free Software Foundation, Inc.
+;;;; Copyright (C) 1999, 2001, 2004, 2005, 2006, 2008 Free Software Foundation, Inc.
 ;;;; 
 ;;;; This program is free software; you can redistribute it and/or modify
 ;;;; it under the terms of the GNU General Public License as published by
@@ -168,7 +168,11 @@
 
   (pass-if-exception "read-only string"
     exception:read-only-string
-    (string-set! (substring/read-only "abc" 0) 1 #\space)))
+    (string-set! (substring/read-only "abc" 0) 1 #\space))
+
+  (pass-if-exception "literal string"
+    exception:read-only-string
+    (string-set! "an immutable string" 0 #\a)))
 
 (with-test-prefix "string-split"
 
-- 
1.6.0

Re: string-set! examples in r5rs.html

[Neil Jerram]

Hi!

2008/9/22 Ludovic Courtès <ludo@gnu.org>:
> Hi,
>
> "Bill Schottstaedt" <bil@ccrma.Stanford.EDU> writes:
>
>> according to r5rs.html, these should signal an error, I believe:
>>
>> guile> (string-set! (symbol->string 'immutable)
>>              0
>>              #\?)
>> guile> (define (g) "***")
>> guile> (string-set! (g) 0 #\?)
>> guile> (g)
>> "?**"
>
> The attached patches against 1.8.x fix this.
>
> Neil: OK to apply?

Yes.  I have a few queries, which could lead to minor changes, but
nothing that important...

-(use-modules (ice-9 documentation))
+(define-module (test-suite test-symbols)
+  #:use-module (test-suite lib)
+  #:use-module (ice-9 documentation))

That's an interesting detail that I haven't noticed before.  (i.e. I
hadn't noticed that some of our test files have a define-module, and
some haven't.)  I've no objection to the define-module, but I'm
curious about why you added it.  If it makes an important difference,
it might be worth commenting.

+  if (SCM_UNLIKELY (STRING_LENGTH (str)) == 0)
+    /* We want the empty string to be `eq?' with the read-only empty
+       string.  */
+    return str;

Completely agree, but I wonder if we should be doing this optimization
in scm_i_substring_read_only() and scm_i_substring(), instead of here?
 Then we'd catch more cases.

> If yes, I would eventually take this opportunity to make
> `make-read-only-string' public.

Well, there is already scm_c_substring_read_only().  It feels like it
would be confusing to add another make-a-read-only-string API.  And if
your "" optimization was in scm_i_substring_read_only(),
scm_c_substring_read_only() would already be fine, wouldn't it?

Finally, while looking through related code, I noticed this in strings.c:

#if SCM_ENABLE_DEPRECATED

/* When these definitions are removed, it becomes reasonable to use
   read-only strings for string literals.  For that, change the reader
   to create string literals with scm_c_substring_read_only instead of
   with scm_c_substring_copy.
*/

...

Is that a concern?  I don't immediately see why these deprecated
functions are supposed to conflict with making literal strings
read-only, but the person who wrote that comment (probably Marius)
seemed to think there would be a conflict...

Regards,
      Neil

Re: string-set! examples in r5rs.html

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 1823 bytes --]

Hello!

"Neil Jerram" <neiljerram@googlemail.com> writes:

> -(use-modules (ice-9 documentation))
> +(define-module (test-suite test-symbols)
> +  #:use-module (test-suite lib)
> +  #:use-module (ice-9 documentation))
>
> That's an interesting detail that I haven't noticed before.  (i.e. I
> hadn't noticed that some of our test files have a define-module, and
> some haven't.)  I've no objection to the define-module, but I'm
> curious about why you added it.  If it makes an important difference,
> it might be worth commenting.

Nothing important: it's just that it's Better(tm) to use separate
namespaces, to avoid side effects among tests.  I've changed some of
them over time.

> Completely agree, but I wonder if we should be doing this optimization
> in scm_i_substring_read_only() and scm_i_substring(), instead of here?
>  Then we'd catch more cases.

Yes, good point.

However, as you noted, it's really an optimization and is not mandated
by R5RS, nor by SRFI-13.  Nevertheless, the `substring/shared' `empty
string' test case in `srfi-13.test' relies on it so it's better to not
break it.

> Finally, while looking through related code, I noticed this in strings.c:
>
> #if SCM_ENABLE_DEPRECATED
>
> /* When these definitions are removed, it becomes reasonable to use
>    read-only strings for string literals.  For that, change the reader
>    to create string literals with scm_c_substring_read_only instead of
>    with scm_c_substring_copy.
> */
>
> ...
>
> Is that a concern?  I don't immediately see why these deprecated
> functions are supposed to conflict with making literal strings
> read-only, but the person who wrote that comment (probably Marius)
> seemed to think there would be a conflict...

I don't see any possible conflicts here.

I committed a revised patch (attached).

Thanks,
Ludo'.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: The patch --]
[-- Type: text/x-patch, Size: 5376 bytes --]

From be5c4a82abb13eb8e00368fe871bcc890a40e97b Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Mon, 22 Sep 2008 23:03:20 +0200
Subject: [PATCH] Make literal strings (i.e., returned by `read') read-only.

* libguile/read.c (scm_read_string): Use `scm_i_make_read_only_string ()' to
  return a read-only string, as mandated by R5RS.  Reported by Bill
  Schottstaedt <bil@ccrma.Stanford.EDU>.

* libguile/strings.c (scm_i_make_read_only_string): New function.
  (scm_i_shared_substring_read_only): Special-case the empty string
  so that the read-only and read-write empty strings are `eq?'.  This
  optimization is relied on by the `substring/shared' `empty string'
  test case in `srfi-13.test'.

* libguile/strings.h (scm_i_make_read_only_string): New declaration.

* test-suite/tests/strings.test ("string-set!")["literal string"]: New test.

* NEWS: Update.
---
 NEWS                          |    1 +
 libguile/read.c               |    2 +-
 libguile/strings.c            |   37 ++++++++++++++++++++++++++++---------
 libguile/strings.h            |    3 ++-
 test-suite/tests/strings.test |    8 ++++++--
 5 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/NEWS b/NEWS
index 9f6e0ed..796da62 100644
--- a/NEWS
+++ b/NEWS
@@ -27,6 +27,7 @@ available: Guile is now always configured in "maintainer mode".
 * Bugs fixed
 
 ** `symbol->string' now returns a read-only string, as per R5RS
+** Literal strings as returned by `read' are now read-only, as per R5RS
 ** `guile-config link' now prints `-L$libdir' before `-lguile'
 ** Fix memory corruption involving GOOPS' `class-redefinition'
 ** Fix possible deadlock in `mutex-lock'
diff --git a/libguile/read.c b/libguile/read.c
index ff50735..9600ecc 100644
--- a/libguile/read.c
+++ b/libguile/read.c
@@ -514,7 +514,7 @@ scm_read_string (int chr, SCM port)
   else
     str = (str == SCM_BOOL_F) ? scm_nullstr : str;
 
-  return str;
+  return scm_i_make_read_only_string (str);
 }
 #undef FUNC_NAME
 
diff --git a/libguile/strings.c b/libguile/strings.c
index 7399d88..ffc1eb3 100644
--- a/libguile/strings.c
+++ b/libguile/strings.c
@@ -218,6 +218,12 @@ get_str_buf_start (SCM *str, SCM *buf, size_t *start)
 }
 
 SCM
+scm_i_make_read_only_string (SCM str)
+{
+  return scm_i_substring_read_only (str, 0, STRING_LENGTH (str));
+}
+
+SCM
 scm_i_substring (SCM str, size_t start, size_t end)
 {
   SCM buf;
@@ -234,15 +240,28 @@ scm_i_substring (SCM str, size_t start, size_t end)
 SCM
 scm_i_substring_read_only (SCM str, size_t start, size_t end)
 {
-  SCM buf;
-  size_t str_start;
-  get_str_buf_start (&str, &buf, &str_start);
-  scm_i_pthread_mutex_lock (&stringbuf_write_mutex);
-  SET_STRINGBUF_SHARED (buf);
-  scm_i_pthread_mutex_unlock (&stringbuf_write_mutex);
-  return scm_double_cell (RO_STRING_TAG, SCM_UNPACK(buf),
-			  (scm_t_bits)str_start + start,
-			  (scm_t_bits) end - start);
+  SCM result;
+
+  if (SCM_UNLIKELY (STRING_LENGTH (str) == 0))
+    /* We want the empty string to be `eq?' with the read-only empty
+       string.  */
+    result = str;
+  else
+    {
+      SCM buf;
+      size_t str_start;
+
+      get_str_buf_start (&str, &buf, &str_start);
+      scm_i_pthread_mutex_lock (&stringbuf_write_mutex);
+      SET_STRINGBUF_SHARED (buf);
+      scm_i_pthread_mutex_unlock (&stringbuf_write_mutex);
+
+      result = scm_double_cell (RO_STRING_TAG, SCM_UNPACK (buf),
+				(scm_t_bits) str_start + start,
+				(scm_t_bits) end - start);
+    }
+
+  return result;
 }
 
 SCM
diff --git a/libguile/strings.h b/libguile/strings.h
index dcc1f11..a7427db 100644
--- a/libguile/strings.h
+++ b/libguile/strings.h
@@ -3,7 +3,7 @@
 #ifndef SCM_STRINGS_H
 #define SCM_STRINGS_H
 
-/* Copyright (C) 1995,1996,1997,1998,2000,2001, 2004, 2005, 2006 Free Software Foundation, Inc.
+/* Copyright (C) 1995,1996,1997,1998,2000,2001, 2004, 2005, 2006, 2008 Free Software Foundation, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -152,6 +152,7 @@ SCM_API void scm_i_get_substring_spec (size_t len,
 				       SCM start, size_t *cstart,
 				       SCM end, size_t *cend);
 SCM_API SCM scm_i_take_stringbufn (char *str, size_t len);
+SCM_API SCM scm_i_make_read_only_string (SCM str);
 
 /* deprecated stuff */
 
diff --git a/test-suite/tests/strings.test b/test-suite/tests/strings.test
index aa9196e..735258a 100644
--- a/test-suite/tests/strings.test
+++ b/test-suite/tests/strings.test
@@ -1,7 +1,7 @@
 ;;;; strings.test --- test suite for Guile's string functions    -*- scheme -*-
 ;;;; Jim Blandy <jimb@red-bean.com> --- August 1999
 ;;;;
-;;;; Copyright (C) 1999, 2001, 2004, 2005, 2006 Free Software Foundation, Inc.
+;;;; Copyright (C) 1999, 2001, 2004, 2005, 2006, 2008 Free Software Foundation, Inc.
 ;;;; 
 ;;;; This program is free software; you can redistribute it and/or modify
 ;;;; it under the terms of the GNU General Public License as published by
@@ -168,7 +168,11 @@
 
   (pass-if-exception "read-only string"
     exception:read-only-string
-    (string-set! (substring/read-only "abc" 0) 1 #\space)))
+    (string-set! (substring/read-only "abc" 0) 1 #\space))
+
+  (pass-if-exception "literal string"
+    exception:read-only-string
+    (string-set! "an immutable string" 0 #\a)))
 
 (with-test-prefix "string-split"
 
-- 
1.6.0

Re: string-set! examples in r5rs.html

[Neil Jerram]

Hi Ludovic,

Thanks for your responses; all make sense to me.

2008/9/23 Ludovic Courtès <ludo@gnu.org>:
>
> I committed a revised patch (attached).

And for the patch!

    Neil

Re: string-set! examples in r5rs.html

[szgyg]

Ludovic Courtes writes:
> "Bill Schottstaedt" writes:
> 
>> according to r5rs.html, these should signal an error, I believe:
>>
>> guile> (string-set! (symbol->string 'immutable)
>>              0
>>              #\?)
>> guile> (define (g) "***")
>> guile> (string-set! (g) 0 #\?)
>> guile> (g)
>> "?**"

Not in R5RS. [1] But R6RS requires reporting. [2]

> The attached patches against 1.8.x fix this.
> 
> Neil: OK to apply?

No, (read) should return mutable string.

"literal constants and the strings returned by symbol->string are 
immutable objects, while all objects created by the other procedures 
listed in this report are mutable." [3]

So scm_read_string() should behave differently when reads program and data.

-------
#;> (string-set! (read) 2 #\X)
"12345"

Backtrace:
In current input:
    1: 0* [string-set! "12345" 2 #\X]

<unnamed port>:1:1: In procedure string-set! in expression (string-set! 
(read) 2 ...):
<unnamed port>:1:1: string is read-only: "12345"
ABORT: (misc-error)
-------

szgyg

[1] 
http://www.schemers.org/Documents/Standards/R5RS/HTML/r5rs-Z-H-4.html#%_sec_1.3.2
[2]
http://www.r6rs.org/final/html/r6rs/r6rs-Z-H-8.html#node_idx_248
[3] 
http://www.schemers.org/Documents/Standards/R5RS/HTML/r5rs-Z-H-6.html#%_idx_76

Re: string-set! examples in r5rs.html

[Ludovic Courtès]

Hi,

szgyg <szgyg@ludens.elte.hu> writes:

> No, (read) should return mutable string.
>
> "literal constants and the strings returned by symbol->string are
> immutable objects, while all objects created by the other procedures
> listed in this report are mutable." [3]
>
> So scm_read_string() should behave differently when reads program and data.

... which doesn't sound convenient, *sigh*.

Technically, strings in Guile all live in writable memory, so it
may actually be wiser to just live with that.

FWIW, the interpreters of Ikarus and Bigloo consider literal strings
writable.

Should we just revert the patch, or does anyone have a better idea?

Thanks,
Ludo'.

Re: string-set! examples in r5rs.html

[Ludovic Courtès]

Hello,

szgyg <szgyg@ludens.elte.hu> writes:

> No, (read) should return mutable string.

I reverted the patch and added a new test case in `readers.test'.

Thanks,
Ludo'.