[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: New lisp library -- cell.el

From: Stefan Monnier
Subject: Re: New lisp library -- cell.el
Date: Fri, 28 Feb 2020 10:43:07 -0500
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux)

Some comments on the code:

> (defsubst cell-option-none ()
>   "Create an option in None variant."
>   cell-option--none-singleton)

Why not use nil for "none"?  That would simplify your code and most
likely would make for much more natural/idiomatic code in the clients of
your library.
[ Traditionally in Lisp when you need something like a "option` (aka
  `Maybe`) type, you use `(cons X nil)` for `some X` and `nil` for `none`.  ]

> (cl-defstruct (cell-box
>                (:constructor nil)
>                (:constructor cell-box--make (-inner))
>                (:copier nil))
>   -inner)

Rather than put the additional "-" in the slot name, you can use
`(:conc-name call-box-)`.

> (defalias 'cell-box-make 'cell-box--make
>   "Create a single mutable unit with INITIAL-VALUE.")
> (defalias 'cell-box-inner 'cell-box--inner
>   "Return the inner value of BOX.
> \n(fn BOX)")

Why bother with those indirections?

> (defun cell-weak-get (cell)
>   "Return Some(inner) if reference in CELL still alive, otherwise return 
> None."
>   (let* ((gensym (make-symbol "not-found"))
>          (val (gethash t (cell-weak--inner-table cell) gensym))
>          (found (not (eq gensym val))))
>     (if found
>         (cell-option-some val)
>       (cell-option-none))))

That makes every call to `cell-weak-get` allocate a new symbol only to
throw it away.  I'd recommend you define a top-level constant

    (defconst cell--not-found (make-symbol "not-found"))

instead.  There are other solutions to this problem that might be even
better, tho.  E.g. you could compute `found` as (< 0 (hash-table-count ...).
Better yet, disallow weak cells containing `nil` (since `nil` can't be
GC'd anyway it doesn't make much sense to put it inside a weak-reference)
and you don't need this at all:

    (defun cell-weak-make (inner)
      "Create a cell takes a weak reference of INNER."
      (if (null inner)
          nil ;; We could also signal an error, but returning nil
              ;; allow us to treat nil as its own weak-reference,
        (let* ((cell (cell-weak--internal-make))
              (internal-ht (cell-weak--inner-table cell)))
          (puthash t inner internal-ht)

    (defun cell-weak-get (cell)
      "Return inner if reference in CELL still alive, otherwise return nil."
      (when cell ;; If nil, it's a weak ref to nil.
        ;; The stored content is never nil, so it only returns nil if
        ;; the content was GC'd.
        (gethash t (cell-weak--inner-table cell))))

-- Stefan

reply via email to

[Prev in Thread] Current Thread [Next in Thread]