Re: [GNU ELPA] New package: tam

[Philip Kaludercic]

Lynn Winebarger <owinebar@gmail.com> writes:

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

So what does that mean?

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

That is true, but I wouldn't bother with maintaining this in the long
term.  Encouraging the usage of apropos commands is good anyway.

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

I am the kind of person who thinks twice about installing a package when
it has dependencies.  But if you prefer to use a package available on
ELPA, then that is of course OK as well.

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

What did you do?  That sounds like something was misquoted:

(defalias 'tam-table-size #'tam--table-size)

?

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

Nothing I say is a requirement, I should have made that explicit.  More
of a "look what you could also do, in case you are interested".

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

In that case, is there a reason you are using setf?

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

Probably.

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

FWIW this is standard practice, and what a cl-loop with collect would
also expand to.  And if I am not mistaken, this is more efficient, than
accumulating a linked list in the right order to begin with (it is a
difference of O(n) vs O(n^2), I believe).

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

Wouldn't that kind of a convenience be an argument against adding an
extra dependency?

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

-- 
Philip Kaludercic