From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Lynn Winebarger Newsgroups: gmane.emacs.devel Subject: Re: [GNU ELPA] New package: tam Date: Mon, 18 Sep 2023 12:22:59 -0400 Message-ID: References: <87a5tjyg83.fsf@posteo.net> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="32022"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel To: Philip Kaludercic Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Mon Sep 18 18:23:35 2023 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qiH27-00084L-Hh for ged-emacs-devel@m.gmane-mx.org; Mon, 18 Sep 2023 18:23:35 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qiH1o-0003G0-1h; Mon, 18 Sep 2023 12:23:16 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qiH1m-0003Fo-EW for emacs-devel@gnu.org; Mon, 18 Sep 2023 12:23:14 -0400 Original-Received: from mail-pg1-x531.google.com ([2607:f8b0:4864:20::531]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qiH1k-0007sn-CK for emacs-devel@gnu.org; Mon, 18 Sep 2023 12:23:14 -0400 Original-Received: by mail-pg1-x531.google.com with SMTP id 41be03b00d2f7-573c62b3cd2so3417141a12.3 for ; Mon, 18 Sep 2023 09:23:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695054191; x=1695658991; darn=gnu.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=ccvgYIWXJ9GVH7nFJELGK+DY7iIB1H5rTGfJ4mLcCqE=; b=G0TKTHl01CcrZikVwPpUVvEADqamIf6Zu03FUEoBZKzqUQis1IBxiKZ/4pyhNNLyit /sesSfvug6sb4x1nVSN283TLE0hpzI7pczu0qF9w6RDMx3Rv2GaiXVv1IMJLS+Df74e5 yTzy3kDcrSyZvGuE/lyBNA5RUc93j0hW7TG1c5AsRSu369z2coERTlnLEitC6T2r+9k1 g8z+C3MbIg0oRxxSUpeTRgNaW72S7td3uRFRYrvnY6EGTVN8urm4NJ0fj0evh6ONBy02 YxRlsgCMODnP03/EA+39Dvtrbr0ae1DWb0HrZamdAUShX5AEC8eejJLfn1IV6MNvriTP juxA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695054191; x=1695658991; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ccvgYIWXJ9GVH7nFJELGK+DY7iIB1H5rTGfJ4mLcCqE=; b=vugsDVWaDNjAtYqEKrp+NVKAdrbOs1yCHD2H2TKljA6GiVn/kGaGkSYb6UNQqlyIgZ G6qO/+2L3evS+/2/t+1r02LhMoLKSz98NuOjU92matrxShvyzXiVogs4NgvkO0m9ix16 FRhHMUzEzAujVlv6WYtvyK9SZmi9IvR/2/8SNKMaXVlmnAzdVNc4HWaUZdkOq9t0xeay +0qNAW51/UMy3lbH4HK6KlVBJo0fefl8KvnzIgUBVlzYYsvd3/uDcaTDuRyiD9RI7933 C8WpzSuJP0j6lV4EVNriLjDASU0aVsYsLgZuHYk7adgL3xdytTLmne4P/BovtyvKICjf zIWA== X-Gm-Message-State: AOJu0Yzx6IhRQmogkWC7GpnL/SikVO4A9y/NiZeZsfRtjxrBdRX5+EmW B1DNdIkWoXlinRKaJnfm5LfcQcujh8j7NNMx8TnlpIBP X-Google-Smtp-Source: AGHT+IF5H60AjWplMgtdhtgQDJTwx0F7SrZU7uOFMwkbNbKFq9keqFUQhWpueoHjPqjbyThXyVo8PKbIsMYcjeI66+g= X-Received: by 2002:a17:90a:d712:b0:274:6f67:e7a8 with SMTP id y18-20020a17090ad71200b002746f67e7a8mr6913820pju.45.1695054190759; Mon, 18 Sep 2023 09:23:10 -0700 (PDT) In-Reply-To: <87a5tjyg83.fsf@posteo.net> Received-SPF: pass client-ip=2607:f8b0:4864:20::531; envelope-from=owinebar@gmail.com; helo=mail-pg1-x531.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:310709 Archived-At: On Mon, Sep 18, 2023 at 5:37=E2=80=AFAM Philip Kaludercic wrote: > > Lynn Winebarger 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 > +;; Maintainer: Onnie Lynn Winebarger > +;; 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 trigger= ing > ;; 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) =3D> table of size N > ;; (tam-table-fullp TABLE) =3D> nil unless TABLE is full > @@ -43,13 +49,12 @@ > ;; (tam-table-free-list TABLE) =3D> list of free indices in TABLE > ;; (tam-table-live-list TABLE) =3D> list of in-use indices in TABLE > > - > ;;; Code: > > (eval-when-compile > (require 'cl-lib)) > > -(require 'queue) > +(require 'queue) ;is this even necessary? see bel= ow. 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." > (<=3D (tam--table-size tbl) (tam--table-used tbl))) > @@ -108,22 +110,20 @@ > "Test if TBL is empty." > (=3D (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 checkd= oc. > "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