From: Mark H Weaver <mhw@netris.org>
To: Andreas Rottmann <a.rottmann@gmx.at>
Cc: guile-devel@gnu.org
Subject: Re: [PATCH] Fix `get-string-n!' &i/o-decoding exception behavior
Date: Sun, 11 Nov 2012 02:26:59 -0500 [thread overview]
Message-ID: <874nkwa9bw.fsf@tines.lan> (raw)
In-Reply-To: <1352332308-5191-1-git-send-email-a.rottmann@gmx.at> (Andreas Rottmann's message of "Thu, 8 Nov 2012 00:51:48 +0100")
Hi Andreas,
Thanks for the patch. See below for my comments.
Andreas Rottmann <a.rottmann@gmx.at> writes:
> Previously, `get-string-n!' from `(rnrs io ports)' would not throw the
> exception required by R6RS, and could not easily do so due to being
> implemented entirely in C.
>
> This change fixes this by introducing a corresponding internal C
> function reporting errors by return value and reimplementing the
> `get-string-n!' in Scheme on top of that. Along with `get-string-n!',
> `get-string-n' gets fixed, inheriting the correct behavior.
>
> * libguile/ports.c (scm_i_getc): New function, a version of `scm_getc'
> not using exceptions.
> (scm_getc): Implemented using `scm_i_getc'.
> * libguile/ports.h (scm_i_getc): Add prototype marked SCM_INTERNAL.
>
> * libguile/r6rs-ports.c (scm_i_get_string_n_x): Exception-free version
> of `get-string-n!', making use of `scm_i_getc'.
> (scm_get_string_n_x): Removed, now implemented in Scheme.
>
> * module/ice-9/binary-ports.scm (get-string-n!): Removed from export
> list, it doesn't fit the module module purpose anyway.
> * module/rnrs/io/ports.scm (%get-string-n): Newly defined by internal
> reference to `(ice-9 binary-ports)'.
> (get-string-n!): Implemented in Scheme on top of `%get-string-n!'.
>
> * test-suite/tests/r6rs-ports.test ("8.2.9 Textual
> input")["read-error"]: Activate commented-out exception-behavior tests
> of `get-string-n!'.
> ["decoding error"]: New test prefix with tests for `get-char',
> `get-string-n!' and `get-string-n' and `get-line'.
> ---
> libguile/ports.c | 20 ++++++++++++++++----
> libguile/ports.h | 1 +
> libguile/r6rs-ports.c | 21 +++++++++++----------
> module/ice-9/binary-ports.scm | 6 +++---
> module/rnrs/io/ports.scm | 14 ++++++++++++++
> test-suite/tests/r6rs-ports.test | 24 ++++++++++++++++++++----
> 6 files changed, 65 insertions(+), 21 deletions(-)
>
> diff --git a/libguile/ports.c b/libguile/ports.c
> index 55808e2..b653af4 100644
> --- a/libguile/ports.c
> +++ b/libguile/ports.c
> @@ -1392,12 +1392,10 @@ scm_t_wchar
> scm_getc (SCM port)
> #define FUNC_NAME "scm_getc"
> {
> - int err;
> - size_t len;
> + int err = 0;
> scm_t_wchar codepoint;
> - char buf[SCM_MBCHAR_BUF_SIZE];
>
> - err = get_codepoint (port, &codepoint, buf, &len);
> + codepoint = scm_i_getc (port, &err);
> if (SCM_UNLIKELY (err != 0))
> /* At this point PORT should point past the invalid encoding, as per
> R6RS-lib Section 8.2.4. */
> @@ -1407,6 +1405,20 @@ scm_getc (SCM port)
> }
> #undef FUNC_NAME
>
> +/* Read a codepoint from PORT and return it. This version reports
> + errors via the ERROR argument instead of via exceptions. */
> +scm_t_wchar
> +scm_i_getc (SCM port, int *error)
> +{
> + size_t len;
> + scm_t_wchar codepoint;
> + char buf[SCM_MBCHAR_BUF_SIZE];
> +
> + *error = get_codepoint (port, &codepoint, buf, &len);
> +
> + return codepoint;
> +}
Given how trivial 'scm_i_getc' is, I think I'd prefer to leave
'scm_getc' alone, to avoid the additional overhead of another non-static
C function call, which has to be done via the procedure linkage table
(PLT) when libguile is a shared library and is thus not entirely
trivial.
> diff --git a/libguile/r6rs-ports.c b/libguile/r6rs-ports.c
> index e867429..bd10081 100644
> --- a/libguile/r6rs-ports.c
> +++ b/libguile/r6rs-ports.c
> @@ -1242,18 +1242,17 @@ SCM_DEFINE (scm_i_make_transcoded_port,
> \f
> /* Textual I/O */
>
> -SCM_DEFINE (scm_get_string_n_x,
> - "get-string-n!", 4, 0, 0,
> +SCM_DEFINE (scm_i_get_string_n_x,
> + "%get-string-n!", 4, 0, 0,
I'm a little bit nervous about this. Although it is not documented in
the manual, 'scm_get_string_n_x' is declared in r6rs-ports.h as SCM_API.
I'm not sure it's safe to make that go away.
Why not leave the API as-is, and in the event of an error, just raise
the proper R6RS exception from within 'scm_get_string_n_x'?
> diff --git a/module/ice-9/binary-ports.scm b/module/ice-9/binary-ports.scm
> index c07900b..3f7b9e6 100644
> --- a/module/ice-9/binary-ports.scm
> +++ b/module/ice-9/binary-ports.scm
> @@ -37,14 +37,14 @@
> get-bytevector-n!
> get-bytevector-some
> get-bytevector-all
> - get-string-n!
Users may have come to rely on this export from (ice-9 binary-ports).
I don't think it's safe to remove it.
Regards,
Mark
next prev parent reply other threads:[~2012-11-11 7:26 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-11-07 23:51 [PATCH] Fix `get-string-n!' &i/o-decoding exception behavior Andreas Rottmann
2012-11-11 7:26 ` Mark H Weaver [this message]
2012-11-11 18:31 ` Andreas Rottmann
2012-11-11 21:20 ` Mark H Weaver
2012-11-11 23:45 ` Mark H Weaver
2012-11-12 19:52 ` Andreas Rottmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/guile/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874nkwa9bw.fsf@tines.lan \
--to=mhw@netris.org \
--cc=a.rottmann@gmx.at \
--cc=guile-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).