bug#10616: flush procedure for soft ports isn't called

bug#10616: flush procedure for soft ports isn't called

[Ian Price]

Hi guilers,

I've noticed when trying to use soft ports that the 3rd procedure in the
vector doesn't get called when I use `force-output'. The simplest
example follows

scheme@(guile−user)> (let* ((port (make-soft-port
                                   (vector #f #f (lambda () 'flushed) #f #f)
                                   "rw")))
                       (force-output port))
scheme@(guile−user)> 

I have no issues with the other procedures. This is on current stable,
on 32 bit x86 running Fedora 16. More complicated examples available on
request.

-- 
Ian Price

"Programming is like pinball. The reward for doing it well is
the opportunity to do it again" - from "The Wizardy Compiled"

bug#10616: flush procedure for soft ports isn't called

[Ian Price]

Ian Price <ianprice90@googlemail.com> writes:

> scheme@(guile−user)> (let* ((port (make-soft-port
>                                    (vector #f #f (lambda () 'flushed) #f #f)
>                                    "rw")))
>                        (force-output port))
> scheme@(guile−user)> 

Yak, this example is a horrible one, since force-output doesn't return
the return value of the procedure anyway, but if you try replacing it
with (lambda () (throw 'wontthrow)), and you'll see it doesn't error

In sf_flush in libguile/vports.c, there is a test
  if (pt->write_pos > pt->write_buf)
which some printf debugging tells me is never true, in fact, those
values never change, no matter how much I write to the port. Some food
for thought while I grapple with how the ports code is supposed to work.

-- 
Ian Price

"Programming is like pinball. The reward for doing it well is
the opportunity to do it again" - from "The Wizardy Compiled"

bug#10616: flush procedure for soft ports isn't called

[Ian Price]

Ian Price <ianprice90@googlemail.com> writes:

> Yak, this example is a horrible one, since force-output doesn't return
> the return value of the procedure anyway, but if you try replacing it
> with (lambda () (throw 'wontthrow)), and you'll see it doesn't error
>
> In sf_flush in libguile/vports.c, there is a test
>   if (pt->write_pos > pt->write_buf)
> which some printf debugging tells me is never true, in fact, those
> values never change, no matter how much I write to the port. Some food
> for thought while I grapple with how the ports code is supposed to work.

OK, so as I understand it, this is supposed to check if there has been a
write to the port. And the buffer will have at most one character in it
because it is set up that way in scm_port_non_buffer.

I notice that sf_write doesn't update this buffer, but instead just
calls the write procedure directly. In that case, it makes sense that
the value isn't changing, and therefore maybe I should just change
sf_flush into a wrapper for the flush procedure?

-- 
Ian Price

"Programming is like pinball. The reward for doing it well is
the opportunity to do it again" - from "The Wizardy Compiled"

bug#10616: flush procedure for soft ports isn't called

[Ian Price]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

Ian Price <ianprice90@googlemail.com> writes:

> I notice that sf_write doesn't update this buffer, but instead just
> calls the write procedure directly. In that case, it makes sense that
> the value isn't changing, and therefore maybe I should just change
> sf_flush into a wrapper for the flush procedure?

I decided to go ahead and do this. Patch attached. Test case missing as
is traditional for soft ports :), though in all seriousness, soft ports
have been around for ages, and this lack of testing for it is, unsettling.

-- 
Ian Price

"Programming is like pinball. The reward for doing it well is
the opportunity to do it again" - from "The Wizardy Compiled"


[-- Attachment #2: ~/src/guile/0001-Fix-flush-on-soft-ports-so-that-it-actually-runs.patch --]
[-- Type: message/external-body, Size: 104 bytes --]

bug#10616: flush procedure for soft ports isn't called

[Ludovic Courtès]

Hi Ian,

I fail to see the patch, and to retrieve it from
<http://bugs.gnu.org/10616>.  Could you resend it inline?

Thanks!

Ludo’.

bug#10616: flush procedure for soft ports isn't called

[Ian Price]

[-- Attachment #1: Type: text/plain, Size: 365 bytes --]

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

> Hi Ian,
>
> I fail to see the patch, and to retrieve it from
> <http://bugs.gnu.org/10616>.  Could you resend it inline?
Bah, that's twice this has happened recently.

-- 
Ian Price

"Programming is like pinball. The reward for doing it well is
the opportunity to do it again" - from "The Wizardy Compiled"


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: soft port patch --]
[-- Type: text/x-patch, Size: 1179 bytes --]

From 8a9524014ce85fb34fe5cfd7a2667395ce0cdf5d Mon Sep 17 00:00:00 2001
From: Ian Price <ianprice90@googlemail.com>
Date: Fri, 27 Jan 2012 06:38:09 +0000
Subject: [PATCH] Fix flush on soft ports, so that it actually runs.

* libguile/vports.c (sf_flush): Remove conditional testing the
  position in the port's write_buf, as it is no longer used.
---
 libguile/vports.c |   18 ++++--------------
 1 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/libguile/vports.c b/libguile/vports.c
index 5178d79..75e7df3 100644
--- a/libguile/vports.c
+++ b/libguile/vports.c
@@ -56,21 +56,11 @@ sf_flush (SCM port)
   scm_t_port *pt = SCM_PTAB_ENTRY (port);
   SCM stream = SCM_PACK (pt->stream);
 
-  if (pt->write_pos > pt->write_buf)
-    {
-      /* write the byte. */
-      scm_call_1 (SCM_SIMPLE_VECTOR_REF (stream, 0),
-		  SCM_MAKE_CHAR (*pt->write_buf));
-      pt->write_pos = pt->write_buf;
-  
-      /* flush the output.  */
-      {
-	SCM f = SCM_SIMPLE_VECTOR_REF (stream, 2);
+  SCM f = SCM_SIMPLE_VECTOR_REF (stream, 2);
+
+  if (scm_is_true (f))
+    scm_call_0 (f);
 
-	if (scm_is_true (f))
-	  scm_call_0 (f);
-      }
-    }
 }
 
 static void
-- 
1.7.7.6

bug#10616: flush procedure for soft ports isn't called

[Mark H Weaver]

Hi Ian,

Ian Price <ianprice90@googlemail.com> writes:
> From 8a9524014ce85fb34fe5cfd7a2667395ce0cdf5d Mon Sep 17 00:00:00 2001
> From: Ian Price <ianprice90@googlemail.com>
> Date: Fri, 27 Jan 2012 06:38:09 +0000
> Subject: [PATCH] Fix flush on soft ports, so that it actually runs.
>
> * libguile/vports.c (sf_flush): Remove conditional testing the
>   position in the port's write_buf, as it is no longer used.
> ---
>  libguile/vports.c |   18 ++++--------------
>  1 files changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/libguile/vports.c b/libguile/vports.c
> index 5178d79..75e7df3 100644
> --- a/libguile/vports.c
> +++ b/libguile/vports.c
> @@ -56,21 +56,11 @@ sf_flush (SCM port)
>    scm_t_port *pt = SCM_PTAB_ENTRY (port);
>    SCM stream = SCM_PACK (pt->stream);
>  
> -  if (pt->write_pos > pt->write_buf)
> -    {
> -      /* write the byte. */
> -      scm_call_1 (SCM_SIMPLE_VECTOR_REF (stream, 0),
> -		  SCM_MAKE_CHAR (*pt->write_buf));
> -      pt->write_pos = pt->write_buf;
> -  
> -      /* flush the output.  */
> -      {
> -	SCM f = SCM_SIMPLE_VECTOR_REF (stream, 2);
> +  SCM f = SCM_SIMPLE_VECTOR_REF (stream, 2);
> +
> +  if (scm_is_true (f))
> +    scm_call_0 (f);
>  
> -	if (scm_is_true (f))
> -	  scm_call_0 (f);
> -      }
> -    }
>  }
>  
>  static void

Applied, thanks!

     Mark

bug#10616: flush procedure for soft ports isn't called

[Ludovic Courtès]

Hello!

Ian Price <ianprice90@googlemail.com> skribis:

>  
> -  if (pt->write_pos > pt->write_buf)
> -    {
> -      /* write the byte. */
> -      scm_call_1 (SCM_SIMPLE_VECTOR_REF (stream, 0),
> -		  SCM_MAKE_CHAR (*pt->write_buf));
> -      pt->write_pos = pt->write_buf;
> -  
> -      /* flush the output.  */
> -      {
> -	SCM f = SCM_SIMPLE_VECTOR_REF (stream, 2);
> +  SCM f = SCM_SIMPLE_VECTOR_REF (stream, 2);
> +
> +  if (scm_is_true (f))
> +    scm_call_0 (f);
>  
> -	if (scm_is_true (f))
> -	  scm_call_0 (f);
> -      }
> -    }
>  }

It’s a bit late to reply (sorry, Ian!), but the reason it took me so
long, is that I wanted to understand the rationale for the ‘if’, and the
implications of dropping it (which I never got around to, as you can
see.  ;-))

Mark: what’s your take on this?  I’m especially concerned with
undesirable side effects in user code.

Thanks,
Ludo’.

bug#10616: flush procedure for soft ports isn't called

[Mark H Weaver]

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

> Ian Price <ianprice90@googlemail.com> skribis:
>
>>  
>> -  if (pt->write_pos > pt->write_buf)
>> -    {
>> -      /* write the byte. */
>> -      scm_call_1 (SCM_SIMPLE_VECTOR_REF (stream, 0),
>> -		  SCM_MAKE_CHAR (*pt->write_buf));
>> -      pt->write_pos = pt->write_buf;
>> -  
>> -      /* flush the output.  */
>> -      {
>> -	SCM f = SCM_SIMPLE_VECTOR_REF (stream, 2);
>> +  SCM f = SCM_SIMPLE_VECTOR_REF (stream, 2);
>> +
>> +  if (scm_is_true (f))
>> +    scm_call_0 (f);
>>  
>> -	if (scm_is_true (f))
>> -	  scm_call_0 (f);
>> -      }
>> -    }
>>  }
>
> It’s a bit late to reply (sorry, Ian!), but the reason it took me so
> long, is that I wanted to understand the rationale for the ‘if’, and the
> implications of dropping it (which I never got around to, as you can
> see.  ;-))
>
> Mark: what’s your take on this?  I’m especially concerned with
> undesirable side effects in user code.

I searched libguile for occurrences of 'write_pos' and 'write_buf', and
convinced myself that Ian's analysis was indeed correct.  The write
buffer is not used by the core ports code.  Writes are forwarded
directly to the write function of the specific port type, which may use
the write buffer if it wishes to, but need not.  The write buffer is
used only by certain types of ports: currently string ports and file
ports.  It is not used by soft ports.

    Thanks,
      Mark

bug#10616: flush procedure for soft ports isn't called

[Ludovic Courtès]

Hi Mark,

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

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Ian Price <ianprice90@googlemail.com> skribis:
>>
>>>  
>>> -  if (pt->write_pos > pt->write_buf)
>>> -    {
>>> -      /* write the byte. */
>>> -      scm_call_1 (SCM_SIMPLE_VECTOR_REF (stream, 0),
>>> -		  SCM_MAKE_CHAR (*pt->write_buf));
>>> -      pt->write_pos = pt->write_buf;
>>> -  
>>> -      /* flush the output.  */
>>> -      {
>>> -	SCM f = SCM_SIMPLE_VECTOR_REF (stream, 2);
>>> +  SCM f = SCM_SIMPLE_VECTOR_REF (stream, 2);
>>> +
>>> +  if (scm_is_true (f))
>>> +    scm_call_0 (f);
>>>  
>>> -	if (scm_is_true (f))
>>> -	  scm_call_0 (f);
>>> -      }
>>> -    }
>>>  }
>>
>> It’s a bit late to reply (sorry, Ian!), but the reason it took me so
>> long, is that I wanted to understand the rationale for the ‘if’, and the
>> implications of dropping it (which I never got around to, as you can
>> see.  ;-))
>>
>> Mark: what’s your take on this?  I’m especially concerned with
>> undesirable side effects in user code.
>
> I searched libguile for occurrences of 'write_pos' and 'write_buf', and
> convinced myself that Ian's analysis was indeed correct.  The write
> buffer is not used by the core ports code.  Writes are forwarded
> directly to the write function of the specific port type, which may use
> the write buffer if it wishes to, but need not.  The write buffer is
> used only by certain types of ports: currently string ports and file
> ports.  It is not used by soft ports.

OK.  Thanks for the detailed analysis!

Ludo’.