bug#10621: Incorrect usage of hash procedures in (ice-9 mapping)

bug#10621: Incorrect usage of hash procedures in (ice-9 mapping)

[Mark H Weaver]

These are genuine errors:

ice-9/mapping.scm:97:48: warning: possibly wrong number of arguments to `hashx-get-handle'
ice-9/mapping.scm:94:48: warning: possibly unbound variable `hashx-create-handle'

     Mark

bug#10621: Incorrect usage of hash procedures in (ice-9 mapping)

[Daniel Hartwig]

Mark H Weaver <mhw@netris.org> wrote:
> ice-9/mapping.scm:97:48: warning: possibly wrong number of arguments to `hashx-get-handle'
> ice-9/mapping.scm:94:48: warning: possibly unbound variable `hashx-create-handle'

This module is quite ancient, and, in addition to these warnings, has
not worked for some time.  Some logic errors in hash-table-mapping
(detecting len) and mapping-ref (detecting dflt), for example.  Since
noone has reported the brokenness I guess that means the module has no
users.

There is no practical way to accomodate a separate delete-proc, as
hashx-remove! uses assoc.  The use of delq! and others to convey the
intended delete semantics is incorrect, since mapping-remove! actually
presents and expects a different interface, that of hashq-remove!.

There is no documentation.

It seems the main purpose of this module is to associate hash and assoc
procedures with a hash table, and so provide a single set of accessors
(rather than one for each of hashq, hashv, etc.).  This is more-or-less
handled by both srfi-69 and rnrs hashtables.

A short module, it is not hard to fix, though given all of the above it
makes sense to simply remove it.

bug#10621: Incorrect usage of hash procedures in (ice-9 mapping)

[Daniel Hartwig]

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

On 28 November 2012 23:56, Daniel Hartwig <mandyke@gmail.com> wrote:
> A short module, it is not hard to fix

I was after a ten minute distraction, so tackled this.  See attached.

[-- Attachment #2: 0001-fix-and-update-ice-9-mapping.patch --]
[-- Type: application/octet-stream, Size: 5332 bytes --]

From 30a4722e4453738a45c6d26201c33856dfb4dab9 Mon Sep 17 00:00:00 2001
From: Daniel Hartwig <mandyke@gmail.com>
Date: Thu, 29 Nov 2012 00:17:26 +0800
Subject: [PATCH] fix and update (ice-9 mapping)

* module/ice-9/mapping.scm (mapping-create-handle!): INIT is required.
  (mapping-ref): Rewrite.  Fix problem with DFLT.
  (hash-table-mapping-hooks): Drop DELETE-PROC, hash-table accessors
  only use ASSOC-PROC.  Add INIT to create-handle hook.  Use correct
  hash-table accessors.
  (make-hash-table-mapping): Drop DELETE-PROC.
  (hash-table-mapping): Rewrite.  Drop DELETE-PROC.
---
 module/ice-9/mapping.scm |   77 +++++++++++++++++++---------------------------
 1 file changed, 32 insertions(+), 45 deletions(-)

diff --git a/module/ice-9/mapping.scm b/module/ice-9/mapping.scm
index 2907a8d..74e98e7 100644
--- a/module/ice-9/mapping.scm
+++ b/module/ice-9/mapping.scm
@@ -50,16 +50,15 @@
 
 (define (mapping-get-handle map key)
   ((mapping-hooks-get-handle (mapping-hooks map)) map key))
-(define (mapping-create-handle! map key . opts)
-  (apply (mapping-hooks-create-handle (mapping-hooks map)) map key opts))
+(define (mapping-create-handle! map key init)
+  ((mapping-hooks-create-handle (mapping-hooks map)) map key init))
 (define (mapping-remove! map key)
   ((mapping-hooks-remove (mapping-hooks map)) map key))
 
-(define (mapping-ref map key . dflt)
+(define* (mapping-ref map key #:optional dflt)
   (cond
-   ((mapping-get-handle map key)	=> cdr)
-   (dflt				=> car)
-   (else				#f)))
+   ((mapping-get-handle map key) => cdr)
+   (else dflt)))
 
 (define (mapping-set! map key val)
   (set-cdr! (mapping-create-handle! map key #f) val))
@@ -70,18 +69,18 @@
   (let ((wrap (lambda (proc) (lambda (1st . rest) (apply proc (mapping-data 1st) rest)))))
 
     (perfect-funcq 17
-		   (lambda (hash-proc assoc-proc delete-proc)
-		     (let ((procs (list hash-proc assoc-proc delete-proc)))
+		   (lambda (hash-proc assoc-proc)
+		     (let ((procs (list hash-proc assoc-proc)))
 		       (cond
-			((equal? procs `(,hashq ,assq ,delq!))
+			((equal? procs `(,hashq ,assq))
 			 (make-mapping-hooks (wrap hashq-get-handle)
 					     (wrap hashq-create-handle!)
 					     (wrap hashq-remove!)))
-			((equal? procs `(,hashv ,assv ,delv!))
+			((equal? procs `(,hashv ,assv))
 			 (make-mapping-hooks (wrap hashv-get-handle)
 					     (wrap hashv-create-handle!)
 					     (wrap hashv-remove!)))
-			((equal? procs `(,hash ,assoc ,delete!))
+			((equal? procs `(,hash ,assoc))
 			 (make-mapping-hooks (wrap hash-get-handle)
 					     (wrap hash-create-handle!)
 					     (wrap hash-remove!)))
@@ -90,39 +89,27 @@
 					      (lambda (table key)
 						(hashx-get-handle hash-proc assoc-proc table key)))
 					     (wrap
-					      (lambda (table key)
-						(hashx-create-handle hash-proc assoc-proc table key)))
+					      (lambda (table key init)
+						(hashx-create-handle! hash-proc assoc-proc table key init)))
 					     (wrap
 					      (lambda (table key)
-						(hashx-get-handle hash-proc assoc-proc delete-proc table key)))))))))))
-
-(define (make-hash-table-mapping table hash-proc assoc-proc delete-proc)
-  (make-mapping (hash-table-mapping-hooks hash-proc assoc-proc delete-proc) table))
-
-(define (hash-table-mapping . options)
-  (let* ((size (or (and options (number? (car options)) (car options))
-		   71))
-	 (hash-proc (or (kw-arg-ref options #:hash-proc) hash))
-	 (assoc-proc (or (kw-arg-ref options #:assoc-proc)
-			 (cond
-			  ((eq? hash-proc hash) assoc)
-			  ((eq? hash-proc hashv) assv)
-			  ((eq? hash-proc hashq) assq)
-			  (else (error 'hash-table-mapping
-				       "Hash-procedure specified with no known assoc function."
-				       hash-proc)))))
-	 (delete-proc (or (kw-arg-ref options #:delete-proc)
-			  (cond
-			   ((eq? hash-proc hash) delete!)
-			   ((eq? hash-proc hashv) delv!)
-			   ((eq? hash-proc hashq) delq!)
-			   (else (error 'hash-table-mapping
-					"Hash-procedure specified with no known delete function."
-					hash-proc)))))
-	 (table-constructor (or (kw-arg-ref options #:table-constructor)
-				(lambda (len) (make-vector len '())))))
-    (make-hash-table-mapping (table-constructor size)
-			     hash-proc
-			     assoc-proc
-			     delete-proc)))
-
+						(hashx-remove! hash-proc assoc-proc table key)))))))))))
+
+(define (make-hash-table-mapping table hash-proc assoc-proc)
+  (make-mapping (hash-table-mapping-hooks hash-proc assoc-proc) table))
+
+(define* (hash-table-mapping #:optional (size 71) #:key
+                             (hash-proc hash)
+                             (assoc-proc
+                              (or (assq-ref `((,hashq . ,assq)
+                                              (,hashv . ,assv)
+                                              (,hash . ,assoc))
+                                            hash-proc)
+                                  (error 'hash-table-mapping
+                                         "Hash-procedure specified with no known assoc function."
+                                         hash-proc)))
+                             (table-constructor
+                              (lambda (len) (make-vector len '()))))
+  (make-hash-table-mapping (table-constructor size)
+                           hash-proc
+                           assoc-proc))
-- 
1.7.10.4

bug#10621: Incorrect usage of hash procedures in (ice-9 mapping)

[Andy Wingo]

On Wed 28 Nov 2012 16:56, Daniel Hartwig <mandyke@gmail.com> writes:

> A short module, it is not hard to fix, though given all of the above it
> makes sense to simply remove it.

I think removing it is the right thing.  Even with your patch it still
won't work because the default "table constructor" is a vector instead
of a hash table.

Regardless of whether it were new or old, I would much prefer to remove
this module than to document it properly, especially given the existence
of alternatives like srfi-69 and rnrs hash tables as you mention.

I will apply your patch (so that the compile warnings go away), add a
deprecation notice, and remove the module in master.

Andy
-- 
http://wingolog.org/