Re: [GNU ELPA] New package: tam

[Philip Kaludercic]

Lynn Winebarger <owinebar@gmail.com> writes:

> I'd like to submit "tam" (table allocation manager) for inclusion in
> GNU ELPA.  The source is available at
> https://github.com/owinebar/emacs-table-allocation-manager

Here are a few comments:

diff --git a/table-allocation-manager.el b/table-allocation-manager.el
index 59a5718..286c9a2 100644
--- a/table-allocation-manager.el
+++ b/table-allocation-manager.el
@@ -3,6 +3,10 @@
 ;; Copyright (C) 2023  Onnie Lynn Winebarger
 
 ;; Author: Onnie Lynn Winebarger <owinebar@gmail.com>
+;; Maintainer: Onnie Lynn Winebarger <owinebar@gmail.com>
+;; URL: https://github.com/owinebar/emacs-table-allocation-manager
+;; Package-Requires: ((emacs "24.4") (queue "0.2"))
+
 ;; Keywords: lisp, tools
 
 ;; This program is free software; you can redistribute it and/or modify
@@ -24,7 +28,9 @@
 ;; table.  All allocation is done during initialization to avoid triggering
 ;; garbage collection during allocation/free operations.
 
-;;  API:
+;;  API: (is it necessary to document the API here?  This has to be
+;;  kept up to date, while a M-x apropos-function tam- RET could avoid
+;;  the issue.)
 ;;
 ;;  (tam-create-table N) => table of size N
 ;;  (tam-table-fullp TABLE) => nil unless TABLE is full
@@ -43,13 +49,12 @@
 ;;  (tam-table-free-list TABLE) => list of free indices in TABLE
 ;;  (tam-table-live-list TABLE) => list of in-use indices in TABLE
 
-
 ;;; Code:
 
 (eval-when-compile
   (require 'cl-lib))
 
-(require 'queue)
+(require 'queue)                       ;is this even necessary?  see below.
 
 (cl-defstruct (tam--table (:constructor tam--table-create (size))
                          (:copier tam--copy-table))
@@ -66,16 +71,15 @@
                                       (table index in-use next previous))
                        (:copier tam--copy-slot))
   "Slot in TAM table"
-  table  ;; table containing this slot
-  index  ;; index of slot in table
-  in-use ;; flag indicating if contents are "live"
-  next   ;; next on list of used/free
-  previous ;; previous on list of used/free
-  contents ;; contents of slot
-  )
+  (table       :documentation "table containing this slot")
+  (index       :documentation "index of slot in table")
+  (in-use      :documentation "flag indicating if contents are live")
+  (next                :documentation "next on list of used/free")
+  (previous    :documentation "previous on list of used/free")
+  (contents    :documentation "contents of slot"))
 
 (defun tam-create-table (N)
-  "Make a tam table of size N."
+  "Make a tam table of size N."                ;"tam table" might not be clear.
   (let ((tbl (tam--table-create N))
        (v (make-vector N nil))
        (N-1 (- N 1))
@@ -98,8 +102,6 @@
     (setf (tam--table-last-free tbl) (aref v N-1))
     tbl))
 
-
-
 (defun tam-table-fullp (tbl)
   "Test if TBL is full."
   (<= (tam--table-size tbl) (tam--table-used tbl)))
@@ -108,22 +110,20 @@
   "Test if TBL is empty."
   (= (tam--table-used tbl) 0))
 
-(defsubst tam-table-size (tbl)
+(defsubst tam-table-size (tbl)         ;why not `defalias'
   "Number of slots in TBL."
   (tam--table-size tbl))
 
-
 (defsubst tam-table-used (tbl)
   "Number of slots of TBL in use."
   (tam--table-used tbl))
 
 (defun tam--table-get-slot (tbl idx)
-  "Get slot IDX of TBL"
+  "Get slot IDX of TBL."
   (aref (tam--table-slots tbl) idx))
 
-
 (defun tam-table-get (tbl idx)
-  "Get contents of slot IDX of TBL"
+  "Get contents of slot IDX of TBL."
   (tam--slot-contents (aref (tam--table-slots tbl) idx)))
 
 (defun tam-allocate (tbl obj)
@@ -133,9 +133,14 @@ Returns index or nil if table is full."
        next idx)
     (when (not (tam-table-fullp tbl))
       (setf (tam--slot-previous s) (tam--table-last-used tbl))
-      (if (tam-table-emptyp tbl)
-         (setf (tam--table-first-used tbl) s)
-       (setf (tam--slot-next (tam--table-last-used tbl)) s))
+      (setf (if (tam-table-emptyp tbl)
+               (tam--table-first-used tbl)
+             (tam--slot-next (tam--table-last-used tbl)))
+           s)
+      (setf (if (tam-table-emptyp tbl)
+               (tam--table-first-used tbl)
+             (tam--slot-next (tam--table-last-used tbl)))
+           s)
       (setf (tam--table-last-used tbl) s)
       (setq next (tam--slot-next s))
       (setf (tam--table-first-free tbl) next)
@@ -151,8 +156,9 @@ Returns index or nil if table is full."
     idx))
 
 (defun tam-free (tbl idx)
-  "Free slot at IDX in TBL.  Returns contents of slot IDX.
-Signals an error if IDX is not in use."
+  "Free slot at IDX in TBL.
+Returns contents of slot IDX.  Signals an error if IDX is not in
+use."
   (let ((s (tam--table-get-slot tbl idx))
        (last-free (tam--table-last-free tbl))
        prev next obj)
@@ -185,17 +191,19 @@ Signals an error if IDX is not in use."
     (cl-decf (tam--table-used tbl))
     obj))
 
+;; you appear to only use the queue to track a list of objects, right?
+;; Why not this then:
 (defun tam-table-free-list (tbl)
-  "Return list of free indices in TBL"
+  "Return list of free indices in TBL."
   (let ((s (tam--table-first-free tbl))
-       (q (queue-create)))
+       (q '()))
     (while s
-      (queue-enqueue q (tam--slot-index s))
+      (push (tam--slot-index s) q)
       (setq s (tam--slot-next s)))
-    (queue-all q)))
+    (nreverse q)))                     ;iff even necessary
 
 (defun tam-table-live-list (tbl)
-  "Return list of live indices in TBL"
+  "Return list of live indices in TBL."
   (let ((s (tam--table-first-used tbl))
        (q (queue-create)))
     (while s

-- 
Philip Kaludercic