Re: [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.1-40-g2252321

Re: [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.1-40-g2252321

[Ludovic Courtès]

Hi Andreas,

Thanks for taking care of this, and thanks for the great doc too!

"Andreas Rottmann" <a.rottmann@gmx.at> writes:

> commit 2252321bb77fe83d98d5bcc9db1c76b914e9dd6a
> Author: Andreas Rottmann <a.rottmann@gmx.at>
> Date:   Sat May 7 23:40:14 2011 +0200
>
>     Make the R6RS simple I/O library use conditions
>     
>     * module/rnrs/io/ports.scm (display): Implement as an
>       exception-converting wrapper around Guile's core display.
>     * module/rnrs/io/simple.scm: Don't export Guile's corresponding core
>       procedures, but use `(rnrs io ports)' instead.  This way, we get the
>       conditions required by R6RS raised.
>     
>     * doc/ref/r6rs.texi (rnrs io simple): Mention that these procedures are
>       supposed to raise R6RS conditions.

Could you add one or more test cases?

Thanks,
Ludo’.

Re: [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.1-40-g2252321

[Andreas Rottmann]

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

> Hi Andreas,
>
> Thanks for taking care of this, and thanks for the great doc too!
>
> "Andreas Rottmann" <a.rottmann@gmx.at> writes:
>
>> commit 2252321bb77fe83d98d5bcc9db1c76b914e9dd6a
>> Author: Andreas Rottmann <a.rottmann@gmx.at>
>> Date:   Sat May 7 23:40:14 2011 +0200
>>
>>     Make the R6RS simple I/O library use conditions
>>     
>>     * module/rnrs/io/ports.scm (display): Implement as an
>>       exception-converting wrapper around Guile's core display.
>>     * module/rnrs/io/simple.scm: Don't export Guile's corresponding core
>>       procedures, but use `(rnrs io ports)' instead.  This way, we get the
>>       conditions required by R6RS raised.
>>     
>>     * doc/ref/r6rs.texi (rnrs io simple): Mention that these procedures are
>>       supposed to raise R6RS conditions.
>
> Could you add one or more test cases?
>
I've started to hack on this, aiming for at least providing tests of a
"control sample" of exception-related behaviors in `(rnrs io simple)'
and parts of `(rnrs io ports)'.  However, as this is already
significantly more than one test, it may take a bit.  I could however
push my existing work at any point, if that's needed for any reason.

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

Re: [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.1-40-g2252321

[Andreas Rottmann]

Andreas Rottmann <a.rottmann@gmx.at> writes:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Hi Andreas,
>>
>> Thanks for taking care of this, and thanks for the great doc too!
>>
>> "Andreas Rottmann" <a.rottmann@gmx.at> writes:
>>
>>> commit 2252321bb77fe83d98d5bcc9db1c76b914e9dd6a
>>> Author: Andreas Rottmann <a.rottmann@gmx.at>
>>> Date:   Sat May 7 23:40:14 2011 +0200
>>>
>>>     Make the R6RS simple I/O library use conditions
>>>     
>>>     * module/rnrs/io/ports.scm (display): Implement as an
>>>       exception-converting wrapper around Guile's core display.
>>>     * module/rnrs/io/simple.scm: Don't export Guile's corresponding core
>>>       procedures, but use `(rnrs io ports)' instead.  This way, we get the
>>>       conditions required by R6RS raised.
>>>     
>>>     * doc/ref/r6rs.texi (rnrs io simple): Mention that these procedures are
>>>       supposed to raise R6RS conditions.
>>
>> Could you add one or more test cases?
>>
> I've started to hack on this, aiming for at least providing tests of a
> "control sample" of exception-related behaviors in `(rnrs io simple)'
> and parts of `(rnrs io ports)'.  However, as this is already
> significantly more than one test, it may take a bit.  I could however
> push my existing work at any point, if that's needed for any reason.
>
I've now pushed a first patch (b6a66c2), in the process fixing two bugs.
These were not directly related to the simple I/O change you referenced,
but located in its base library `(rnrs io ports)'.  There's still more to
come in that area:

- Tests for en/decoding error behavior.

- Fixing `get-string-n!' to raise proper conditions.  This also entails
  introducing an extension to scm_getc() which communicates errors via
  return value or output arguments, thus speeding up `get-char'.  I'll
  post a patch for review once I have something worth showing.

I think we are approaching completeness and correctness of our R6RS I/O
support -- besides Section 8.2.13 "Input/output ports" and the EOL
handling, I'm not aware of major stuff missing.

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

Re: [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.1-40-g2252321

[Ludovic Courtès]

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

Hi Andreas,

(Sorry for the laaaate reply, I'm just slowly catching up.)

Andreas Rottmann <a.rottmann@gmx.at> skribis:

> Andreas Rottmann <a.rottmann@gmx.at> writes:
>
>> ludo@gnu.org (Ludovic Courtès) writes:
>>
>>> Hi Andreas,
>>>
>>> Thanks for taking care of this, and thanks for the great doc too!
>>>
>>> "Andreas Rottmann" <a.rottmann@gmx.at> writes:
>>>
>>>> commit 2252321bb77fe83d98d5bcc9db1c76b914e9dd6a
>>>> Author: Andreas Rottmann <a.rottmann@gmx.at>
>>>> Date:   Sat May 7 23:40:14 2011 +0200
>>>>
>>>>     Make the R6RS simple I/O library use conditions
>>>>     
>>>>     * module/rnrs/io/ports.scm (display): Implement as an
>>>>       exception-converting wrapper around Guile's core display.
>>>>     * module/rnrs/io/simple.scm: Don't export Guile's corresponding core
>>>>       procedures, but use `(rnrs io ports)' instead.  This way, we get the
>>>>       conditions required by R6RS raised.
>>>>     
>>>>     * doc/ref/r6rs.texi (rnrs io simple): Mention that these procedures are
>>>>       supposed to raise R6RS conditions.
>>>
>>> Could you add one or more test cases?
>>>
>> I've started to hack on this, aiming for at least providing tests of a
>> "control sample" of exception-related behaviors in `(rnrs io simple)'
>> and parts of `(rnrs io ports)'.  However, as this is already
>> significantly more than one test, it may take a bit.  I could however
>> push my existing work at any point, if that's needed for any reason.
>>
> I've now pushed a first patch (b6a66c2), in the process fixing two bugs.
> These were not directly related to the simple I/O change you referenced,
> but located in its base library `(rnrs io ports)'.


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

+(define (with-i/o-port-error port make-primary-condition thunk)
+  (with-throw-handler 'system-error
+      thunk
+    (lambda args
+      (let ((errno (system-error-errno args)))
+        (if (memv errno (list EIO EFBIG ENOSPC EPIPE))
+            (raise (condition (make-primary-condition)
+                              (make-i/o-port-error port)))
+            (apply throw args))))))
+
+(define-syntax with-textual-output-conditions
+  (syntax-rules ()
+    ((_ port body0 body ...)
+     (with-i/o-port-error port make-i/o-write-error
+       (lambda () (with-i/o-encoding-error body0 body ...))))))

[...]

 (define (put-char port char)
-  (with-i/o-encoding-error (write-char char port)))
+  (with-textual-output-conditions port (write-char char port)))
 
 (define (put-datum port datum)
-  (with-i/o-encoding-error (write datum port)))
+  (with-textual-output-conditions port (write datum port)))

[-- Attachment #3: Type: text/plain, Size: 1300 bytes --]


I'm a bit concerned about the performance implications of the above
change: ‘put-char’, ‘put-datum’, etc. now expand to something like

     (with-i/o-port-error p xxx
       (lambda ()
         (with-throw-handler 'decoding-error
           (lambda ()
             (write-char c p))
           (lambda (key subr message errno port)
             (raise (make-i/o-decoding-error port))))))

So there are at least 4 additional function calls (2
‘with-throw-handler’ calls, and 2 anonymous closure calls.)

Did you do any measurements?  Would be nice to add micro-benchmarks
under benchmark-suite/.

One optimization would be to instead do something such that ‘put-char’
would expand to something like:

  (define (put-char p c)
    (with-throw-handler #t
           (lambda ()
             (write-char c p))
           (lambda args
             (case (car args)
               ((decoding-error)
                (raise (make-i/o-decoding-error p)))
               ((system-error)
                (if (memv (system-error-errno args)
                          `(,EIO ,EFBIG ,ENOSPC ,EPIPE))
                    ...))
               (else
                ;; Not for us.
                (apply throw args))))))

What do you think?

Thanks,
Ludo’.

Re: [Guile-commits] GNU Guile branch, stable-2.0, updated. v2.0.1-40-g2252321

[Andreas Rottmann]

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

> Hi Andreas,
>
> (Sorry for the laaaate reply, I'm just slowly catching up.)
>
> Andreas Rottmann <a.rottmann@gmx.at> skribis:
>
>> Andreas Rottmann <a.rottmann@gmx.at> writes:
>>
>>> ludo@gnu.org (Ludovic Courtès) writes:
>>>
>>>> Hi Andreas,
>>>>
>>>> Thanks for taking care of this, and thanks for the great doc too!
>>>>
>>>> "Andreas Rottmann" <a.rottmann@gmx.at> writes:
>>>>
>>>>> commit 2252321bb77fe83d98d5bcc9db1c76b914e9dd6a
>>>>> Author: Andreas Rottmann <a.rottmann@gmx.at>
>>>>> Date:   Sat May 7 23:40:14 2011 +0200
>>>>>
>>>>>     Make the R6RS simple I/O library use conditions
>>>>>     
>>>>>     * module/rnrs/io/ports.scm (display): Implement as an
>>>>>       exception-converting wrapper around Guile's core display.
>>>>>     * module/rnrs/io/simple.scm: Don't export Guile's corresponding core
>>>>>       procedures, but use `(rnrs io ports)' instead.  This way, we get the
>>>>>       conditions required by R6RS raised.
>>>>>     
>>>>>     * doc/ref/r6rs.texi (rnrs io simple): Mention that these procedures are
>>>>>       supposed to raise R6RS conditions.
>>>>
>>>> Could you add one or more test cases?
>>>>
>>> I've started to hack on this, aiming for at least providing tests of a
>>> "control sample" of exception-related behaviors in `(rnrs io simple)'
>>> and parts of `(rnrs io ports)'.  However, as this is already
>>> significantly more than one test, it may take a bit.  I could however
>>> push my existing work at any point, if that's needed for any reason.
>>>
>> I've now pushed a first patch (b6a66c2), in the process fixing two bugs.
>> These were not directly related to the simple I/O change you referenced,
>> but located in its base library `(rnrs io ports)'.
>
> +(define (with-i/o-port-error port make-primary-condition thunk)
> +  (with-throw-handler 'system-error
> +      thunk
> +    (lambda args
> +      (let ((errno (system-error-errno args)))
> +        (if (memv errno (list EIO EFBIG ENOSPC EPIPE))
> +            (raise (condition (make-primary-condition)
> +                              (make-i/o-port-error port)))
> +            (apply throw args))))))
> +
> +(define-syntax with-textual-output-conditions
> +  (syntax-rules ()
> +    ((_ port body0 body ...)
> +     (with-i/o-port-error port make-i/o-write-error
> +       (lambda () (with-i/o-encoding-error body0 body ...))))))
>
> [...]
>
>  (define (put-char port char)
> -  (with-i/o-encoding-error (write-char char port)))
> +  (with-textual-output-conditions port (write-char char port)))
>  
>  (define (put-datum port datum)
> -  (with-i/o-encoding-error (write datum port)))
> +  (with-textual-output-conditions port (write datum port)))
>
> I'm a bit concerned about the performance implications of the above
> change: ‘put-char’, ‘put-datum’, etc. now expand to something like
>
>      (with-i/o-port-error p xxx
>        (lambda ()
>          (with-throw-handler 'decoding-error
>            (lambda ()
>              (write-char c p))
>            (lambda (key subr message errno port)
>              (raise (make-i/o-decoding-error port))))))
>
> So there are at least 4 additional function calls (2
> ‘with-throw-handler’ calls, and 2 anonymous closure calls.)
>
Yeah, this is bound to be slow, at least for something as basic as
`put-char' -- but correctness trumps performance, right? ;-).

> Did you do any measurements?  Would be nice to add micro-benchmarks
> under benchmark-suite/.
>
No, but I'm planning to add some benchmarks, and do some (basic)
performance work in this area; as my Guiling time is quite limited ATM,
don't hold your breath, though.

> One optimization would be to instead do something such that ‘put-char’
> would expand to something like:
>
>   (define (put-char p c)
>     (with-throw-handler #t
>            (lambda ()
>              (write-char c p))
>            (lambda args
>              (case (car args)
>                ((decoding-error)
>                 (raise (make-i/o-decoding-error p)))
>                ((system-error)
>                 (if (memv (system-error-errno args)
>                           `(,EIO ,EFBIG ,ENOSPC ,EPIPE))
>                     ...))
>                (else
>                 ;; Not for us.
>                 (apply throw args))))))
>
> What do you think?
>
Yep, good idea.  I'll consider this when I have time to put some work
into it.

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