[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [GNU ELPA] New package: tam
From: |
Lynn Winebarger |
Subject: |
Re: [GNU ELPA] New package: tam |
Date: |
Mon, 18 Sep 2023 12:22:59 -0400 |
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
- [GNU ELPA] New package: tam, Lynn Winebarger, 2023/09/17
- Re: [GNU ELPA] New package: tam, Philip Kaludercic, 2023/09/18
- Re: [GNU ELPA] New package: tam,
Lynn Winebarger <=
- Re: [GNU ELPA] New package: tam, Philip Kaludercic, 2023/09/18
- Re: [GNU ELPA] New package: tam, Lynn Winebarger, 2023/09/19
- Re: [GNU ELPA] New package: tam, Philip Kaludercic, 2023/09/20
- Re: [GNU ELPA] New package: tam, Lynn Winebarger, 2023/09/20
- Re: [GNU ELPA] New package: tam, Stefan Monnier, 2023/09/20
- Re: [GNU ELPA] New package: tam, Lynn Winebarger, 2023/09/21
- Re: [GNU ELPA] New package: tam, Stefan Monnier, 2023/09/21
- Re: [GNU ELPA] New package: tam, Lynn Winebarger, 2023/09/21
- Re: [GNU ELPA] New package: tam, Stefan Monnier, 2023/09/21
- Re: [GNU ELPA] New package: tam, Philip Kaludercic, 2023/09/21