Hi Andreas,
Thanks for the patch. See below for my comments.
Andreas Rottmann <a.rottm...@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,
>
> /* 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
