Re: [PATCH 2/4] Add implementation of "transcoded ports"

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

ludo@gnu.org (Ludovic Courtès) writes:

> Hi!
>
> Andreas Rottmann <a.rottmann@gmx.at> writes:
>
>> * libguile/r6rs-ports.c (make_tp, tp_write, tp_fill_input, tp_flush,
>>   tp_close, initialize_transcoded_ports, scm_transcoded_port): New
>>   functions.
>>   (scm_init_r6rs_ports): Call `initialize_transcoded_ports'.
>> * module/rnrs/ports.scm (transcoded-port): Remove, this is now
>>   implemented in C.
>> * test-suite/tests/r6rs-ports.test (8.2.6 Input and output ports): Added a
>>   few tests for `transcoded-port'.
>
> Great!  This looks good to me, modulo the minor things below:
>
>> +  /* We can't use scm_c_read() here, since it blocks until the whole
>> +     block has been read or EOF */
>
> Please write it “`scm_c_read'” and add a period at the end.
>
Done.

>> +SCM_DEFINE (scm_transcoded_port,
>> +	    "transcoded-port", 2, 0, 0,
>> +	    (SCM port, SCM transcoder),
>> +	    "")
>
> Docstring please.  :-)
>
Added.

>> +  SCM_VALIDATE_STRUCT (SCM_ARG1, transcoder);
>
> This type check is too weak.
>
I've restructured the code now so the type checking is done (implictly)
in Scheme.

>> +  /* SCM_CLR_PORT_OPEN_FLAG (port); */
>
> Meaning of this comment?
>
I've replaced it with a (hopefully) more meaningful comment.

>> +(with-test-prefix "8.2.6  Input and output ports"
>> +  (pass-if "transcoded-port [output]"
>> +    (let ((s "Hello\n\304\326\334"))
>
> It seems that it’s not actual UTF-8, or maybe the message mangled it
> somehow?
>
The file has a header which contains this: "coding: iso-8859-1;".  The
strings in the source should hence (IIUC) be in latin-1 encoding -- at
least that's what they are in my git.

>> +           (call-with-port (transcoded-port bv-port (make-transcoder (utf-8-codec)))
>
> I think you forgot the patch that adds ‘make-transcoder’ and
> ‘utf-8-codec’.  :-)
>
As I split the patch into the series, I obviously got the ordering
wrong; sorry.  The "transcoded-port" patch is now the last in the
series.

> Can you send an updated patch (or pair of patches)?
>
I'll send the updated series as follow-up to this mail.

Regards, Rotty
-- 
Andreas Rottmann -- <http://rotty.yi.org/>