Re: [PATCH] Fix `get-string-n!' &i/o-decoding exception behavior

[Mark H Weaver]

Hi Andreas,

Thanks for the patch.  See below for my comments.

Andreas Rottmann <address@hidden> 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