From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: ludo@gnu.org (Ludovic =?iso-8859-1?Q?Court=E8s?=) Newsgroups: gmane.lisp.guile.bugs Subject: Re: string-set! examples in r5rs.html Date: Tue, 23 Sep 2008 18:54:18 +0200 Message-ID: <87hc86n6px.fsf@gnu.org> References: <20080919203017.M86297@ccrma.Stanford.EDU> <873ajr6gcx.fsf@gnu.org> <49dd78620809221550x77dc5a09o44f2a49302d9dca8@mail.gmail.com> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: ger.gmane.org 1222188897 5936 80.91.229.12 (23 Sep 2008 16:54:57 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 23 Sep 2008 16:54:57 +0000 (UTC) To: bug-guile@gnu.org Original-X-From: bug-guile-bounces+guile-bugs=m.gmane.org@gnu.org Tue Sep 23 18:55:55 2008 Return-path: Envelope-to: guile-bugs@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1KiBAt-0004gK-Kk for guile-bugs@m.gmane.org; Tue, 23 Sep 2008 18:55:44 +0200 Original-Received: from localhost ([127.0.0.1]:42205 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KiB9r-00043v-PI for guile-bugs@m.gmane.org; Tue, 23 Sep 2008 12:54:39 -0400 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KiB9o-00043g-4e for bug-guile@gnu.org; Tue, 23 Sep 2008 12:54:36 -0400 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KiB9n-00043U-1O for bug-guile@gnu.org; Tue, 23 Sep 2008 12:54:35 -0400 Original-Received: from [199.232.76.173] (port=42626 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KiB9m-00043R-Sy for bug-guile@gnu.org; Tue, 23 Sep 2008 12:54:34 -0400 Original-Received: from main.gmane.org ([80.91.229.2]:45010 helo=ciao.gmane.org) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1KiB9m-0008B5-0Y for bug-guile@gnu.org; Tue, 23 Sep 2008 12:54:34 -0400 Original-Received: from list by ciao.gmane.org with local (Exim 4.43) id 1KiB9h-0006f1-Jp for bug-guile@gnu.org; Tue, 23 Sep 2008 16:54:29 +0000 Original-Received: from reverse-83.fdn.fr ([80.67.176.83]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Tue, 23 Sep 2008 16:54:29 +0000 Original-Received: from ludo by reverse-83.fdn.fr with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Tue, 23 Sep 2008 16:54:29 +0000 X-Injected-Via-Gmane: http://gmane.org/ Original-Lines: 223 Original-X-Complaints-To: usenet@ger.gmane.org X-Gmane-NNTP-Posting-Host: reverse-83.fdn.fr X-URL: http://www.fdn.fr/~lcourtes/ X-Revolutionary-Date: 2 =?iso-8859-1?Q?Vend=E9miaire?= an 217 de la =?iso-8859-1?Q?R=E9volution?= X-PGP-Key-ID: 0xEA52ECF4 X-PGP-Key: http://www.fdn.fr/~lcourtes/ludovic.asc X-PGP-Fingerprint: 821D 815D 902A 7EAB 5CEE D120 7FBA 3D4F EB1F 5364 X-OS: i686-pc-linux-gnu User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.3 (gnu/linux) Cancel-Lock: sha1:qlrujS5tqAwaI0PsoX/FJ1jkJrI= X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6, seldom 2.4 (older, 4) X-BeenThere: bug-guile@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Bug reports for GUILE, GNU's Ubiquitous Extension Language" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: bug-guile-bounces+guile-bugs=m.gmane.org@gnu.org Errors-To: bug-guile-bounces+guile-bugs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.bugs:4021 Archived-At: --=-=-= Hello! "Neil Jerram" 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'. --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0001-Make-literal-strings-i.e.-returned-by-read-read.patch Content-Description: The patch >From be5c4a82abb13eb8e00368fe871bcc890a40e97b Mon Sep 17 00:00:00 2001 From: =?utf-8?q?Ludovic=20Court=C3=A8s?= 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 . * 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 --- 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 --=-=-=--