Re: [PATCH] Add SRFI-25 implementation

[Mark H Weaver <mhw@netris.org>]

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

> ludo@gnu.org (Ludovic Courtès) skribis:
>
>> Andreas Rottmann <a.rottmann@gmx.at> skribis:
>>
>>> Adds an implementation of SRFI 25 on top of Guile's native arrays. The
>>> implementation does not introduce a disjoint type; Guile arrays and
>>> SRFI-25 arrays can be used interchangably, though with different, partly
>>> conflicting APIs.
>>>
>>> * NEWS: Add preliminary, incomplete section on 2.0.12, noting the
>>>   addition of SRFI-25.
>>> * doc/ref/srfi-modules.texi (SRFI-25): New node.
>>> * module/srfi/srfi-25.scm: New file.
>>> * test-suite/tests/srfi-25.test: New file.
>>> * module/Makefile.am:
>>> * test-suite/Makefile.am: Add new files.
>>> ---
>>>  NEWS                          |  15 +-
>>>  doc/ref/srfi-modules.texi     | 240 +++++++++++++++++++++-
>>>  module/Makefile.am            |   1 +
>>>  module/srfi/srfi-25.scm       | 159 +++++++++++++++
>>>  test-suite/Makefile.am        |   3 +-
>>>  test-suite/tests/srfi-25.test | 461 ++++++++++++++++++++++++++++++++++++++++++
>>>  6 files changed, 876 insertions(+), 3 deletions(-)
>>>  create mode 100644 module/srfi/srfi-25.scm
>>>  create mode 100644 test-suite/tests/srfi-25.test
>>
>> If there are no objections, you can push this patch within a day or two
>> to ‘stable-2.0’ (or I’ll do it if you happen to be unavailable.)
>
> Done in aaea5b2 with minor modifications in srfi-modules.texi to make it
> more consistent with the rest of the manual.

This commit broke the build, because it forgot to add srfi-25.scm.

For now, I reverted the commit instead of fixing it, because there are a
few other problems with srfi-25.scm which I had told Andreas about over
IRC but never posted about.  Sorry about that.  I guess maybe that's why
Andreas didn't respond more quickly to your earlier email.

In a couple of places, it has code like this:

+     (cond ((array? i)
+            (apply array-ref array (array->index-list i)))
+           ((vector? i)
+            (apply array-ref array (vector->list i)))

Since every vector is also considered an array, the second clause of the
'cond' is dead code.

Also, it would be good to avoid consing in common cases in 'array-ref'
and 'array-set!', by adding special cases for 1 and 2 dimensional array
accesses, at least when the indices are given as direct arguments.

Finally, the following line should be put somewhere in the module:

(cond-expand-provide (current-module) '(srfi-25))

     Thanks,
       Mark