Re: [GNU ELPA] New package: tam

[Lynn Winebarger <owinebar@gmail.com>]

On Mon, Sep 18, 2023 at 5:37 AM Philip Kaludercic <philipk@posteo.net> wrote:
>
> 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:
Thanks for taking a look.

>
> 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"))
> +

Apologies, I renamed the library to tam.el and failed to note the
changes I made to the renamed file did not get committed and pushed.

>  ;; 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.)

I thought it would be helpful to summarize the functionality for review.

>  ;;
>  ;;  (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.

If it's a big deal, then no.  Since queue is a GNU package, I
preferred to not repeat code.

>
>  (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'

I tried defalias first, but got a byte-compiler error about a void
variable.  Which I found confusing, since it should be looking for a
function definition, not a variable.  I'm using 28.3.
Some of the following should have already been fixed from when I ran checkdoc.

>    "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)

Is this a personal stylistic preference, or a requirement?  I'll
change it if required, but I find computing the place inside a set
form to be disconcerting if it's not required.  For example, I
wouldn't use a set form like
(set (if P 'A 'B) some-value)
in place of
(if P (setq A some-value) (setq B some-value))

where I might be amenable to
(set (opaque-function-call args ...) some-value)

> +      (setf (if (tam-table-emptyp tbl)
> +               (tam--table-first-used tbl)
> +             (tam--slot-next (tam--table-last-used tbl)))
> +           s)
I'm assuming this is a typo.

>        (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

I do want to return lists reflecting the ordering of the slots.  I am
not a fan of constructing an ordered structure only to reverse it.
I can rewrite this to append to the tail using let-bound head and tail
variables, but it seems excessive to avoid a single allocation of a
queue structure.
That said, these functions are primarily intended for debugging.
>
>  (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