Mark H Weaver <m...@netris.org> writes:
> Hi Andreas,
>
> Thanks for the patch. See below for my comments.
>
> Andreas Rottmann <a.rottm...@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,
>>
>> /* 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/>
