From: Andreas Rottmann <a.rottmann@gmx.at>
To: Mark H Weaver <mhw@netris.org>
Cc: guile-devel@gnu.org
Subject: Re: [PATCH] Fix `get-string-n!' &i/o-decoding exception behavior
Date: Sun, 11 Nov 2012 19:31:23 +0100 [thread overview]
Message-ID: <87haow8004.fsf@delenn.home.rotty.xx.vu> (raw)
In-Reply-To: <874nkwa9bw.fsf@tines.lan> (Mark H. Weaver's message of "Sun, 11 Nov 2012 02:26:59 -0500")
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/>
next prev parent reply other threads:[~2012-11-11 18:31 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
2012-11-11 18:31 ` Andreas Rottmann [this message]
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=87haow8004.fsf@delenn.home.rotty.xx.vu \
--to=a.rottmann@gmx.at \
--cc=guile-devel@gnu.org \
--cc=mhw@netris.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).