Re: Making custom binary input ports unbuffered

[Mark H Weaver]

Hi Ludovic,

Thanks for the quick review!

address@hidden (Ludovic Courtès) writes:

> Mark H Weaver <address@hidden> skribis:
>
>> Here are the same changes adapted for master, where we can put the
>> new 'setvbuf' method where it belongs: in the PTOB.
>
> Cool!
>
> Wouldn’t it be easier to just merge stable-2.0 and modify from there?

That's initially what I started to do, but merging this patch to master
was more work than I expected.  There are massive changes to the ports
code in master.  It involved asking questions like "Are there any new
ways to create file ports in master?", and involved several merge
failures for changes that will be immediately and completely undone
anyway.  I found myself asking "Why am I doing this?"

If you want to do it like you suggest, would you be willing to handle
the merge of your setvbuf patches to master?  I've already merged
everything else.

Anyway, please see below for additional responses to your comments.

>> From 00ee913e2da658f30d9d8682edfbb9a63719c370 Mon Sep 17 00:00:00 2001
>> From: Mark H Weaver <address@hidden>
>> Date: Tue, 21 Jan 2014 01:57:31 -0500
>> Subject: [PATCH 1/2] Prepare 'setvbuf' to support for non-file ports.
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>>
>> Based on a patch for Guile 2.0 by Ludovic Courtès.
>>
>> * libguile/fports.c (scm_fport_buffer_add): Rename to 'fport_setvbuf'.
>>   (fport_setvbuf): Renamed from 'scm_fport_buffer_add'.  Change type of
>>   'write_size' argument from 'int' to 'long'.
>>   (scm_i_fdes_to_port): Adapt to renamed 'scm_fport_buffer_add'.
>>   (scm_make_fptob): Set 'setvbuf' method to 'fport_setvbuf'.
>>   (scm_setvbuf): Move to ports.c.
>>
>> * libguile/ports.c (scm_make_port_type): Initialize 'setvbuf' field.
>>   (scm_set_port_setvbuf): New API function.
>>   (scm_setvbuf): Moved from fports.c.  Accept any open port, and error
>>   out when the ptob's 'setvbuf' field is NULL.  Remove explicit
>>   'scm_gc_free' calls.  Call ptob's 'setvbuf' method instead of
>>   'scm_fport_buffer_add'.
>>
>> * libguile/fports.h (scm_setbuf0): Remove prototype for non-existent
>>   function.
>>   (scm_setvbuf): Move prototype to ports.h.
>>
>> * libguile/ports.h (scm_t_ptob_descriptor): Add 'setvbuf' member.
>>   (scm_set_port_setvbuf): Add prototype.
>>   (scm_setvbuf): Move prototype here from fports.h.
>
> Moving ‘setvbuf’ to ports.c also needs to be done in stable-2.0 (and
> it’s worth making it in a separate patch, IMO.)

Okay.  Other changes that could be done in 2.0 include renaming
'scm_fport_buffer_add' to 'fport_setvbuf' and removing the prototype for
'scm_setbuf0'.

>
>> * libguile/ports-internal.h (struct scm_port_internal): Change
>>   'pending_eof' to a 1-bit unsigned char.  Add comment for 'alist'
>>   member.
>>
>> * test-suite/tests/ports.test ("setvbuf")["closed port", "string port"]:
>>   New tests.
>>
>> * doc/ref/api-io.texi (Port Implementation): Document new 'setvbuf'
>>   member of ptob, and 'scm_set_port_setvbuf'.
>
> I was wondering if the ‘setvbuf’ method should be exposed at this point,
> because I wasn’t 100% sure of the API.  WDYT?

Hmm.  I'm not 100% sure of it either.  Unfortunately, we cannot avoid
exposing it as soon as it goes into the PTOB, since the PTOB structure
is public.

Can we discuss the API now and reach a decision?

> Perhaps it’s also better for a separate patch?

What as a separate patch?  Do you mean that we should augment the PTOB
in one patch and not document it until a later patch?

    Thanks!
      Mark