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

[Andreas Rottmann <a.rottmann@gmx.at>]

Mark H Weaver <mhw@netris.org> writes:

> Hi Andreas,
>
> Thanks for the patch.  See below for my comments.
>
> Andreas Rottmann <a.rottmann@gmx.at> writes:
>
>> 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.
>
Yup, that's fine with me.

>> 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.
>
Oh, I missed that one; thanks.

> 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'?
>
The problem here is that we have no easy way to raise R6RS exceptions
from C code, AFAICT.  It is certainly possible, but if it involves
convoluted code of doing imports of condition types and appropriate
constructors, then constructing a proper invocation, all in C, I'd
rather avoid it.  I think the tendency (in general, also in Guile's
implementation) was to do more things in Scheme, and less in C.

However, exceptions are a difficult topic; if we want efficient
(i.e. ones not requiring setting up exception converters on each call)
implementations of R6RS I/O procedures eventually, we'd either

(a) need to expose exception-less primitives (like I attempted with
    scm_i_get_c and scm_i_get_string_n_x), and use those to implement
    the actual exception-throwing procedures in Scheme, or

(b) if you really want this done (or doable) fully in C, I think we'd
    first provide an API (at least an internal one), to make it possible
    to easily raise R6RS conditions from C.

In that sense, it's unfortunate that r6rs-ports.h is public API at all,
since it effectively prevents us from using strategy (a), at least
without going through a deprecation phase. These functions are now all
broken wrt. exceptions -- do we want to fix this at the C level as well,
or rather deprecate them, and DTRT (only) for their Scheme counterparts?

>> 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.
>
Hmm, yeah, that has bothered me a bit as well, but I forgot to
explicitly point it out making the patch potentially unsuitable for
stable.  Nevertheless, having textual I/O procedures in `(ice-9
binary-ports)' seems quite a bit strange; maybe leave it there (for
stable-2.0) but mark that binding as deprecated -- if users want R6RS
textual I/O, they know where to find it ;-). 

Also, I'm not sure if procedures in `(ice-9 ...)' can be expected to be
throw R6RS conditions at all...

Regards, Rotty
-- 
Andreas Rottmann -- <http://rotty.xx.vu/>