Re: Separating obarray handling from abbrevs

[Przemysław Wojnowski]

Nicolas Petton <address@hidden> writes:

> Przemysław Wojnowski <address@hidden> writes:
>
>> +;;; obarray-lib.el --- obarray functions -*- lexical-binding: t -*-
>
> Why `obarray-lib' and not simply `obarray'?  AFAIK very few library
> names are postfixed with `-lib'.  I think it would be a nicer package
> name, and it would be consistent with the prefix of your functions.
>
> Nico

Thanks for the feedback!

Here are corrected versions.

>From 2dabe20527a3b53d345006648cca89522f4eff3e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Przemys=C5=82aw=20Wojnowski?= <address@hidden>
Date: Sun, 8 Nov 2015 16:59:07 +0100
Subject: [PATCH 1/2] New file with obarray functions.

* lisp/obarray.el: basic obarray functions extracted from abbrev.el
* test/automated/obarray-tests.el: new file
---
 lisp/obarray.el                 | 65 +++++++++++++++++++++++++++++
 test/automated/obarray-tests.el | 90 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 155 insertions(+)
 create mode 100644 lisp/obarray.el
 create mode 100644 test/automated/obarray-tests.el

diff --git a/lisp/obarray.el b/lisp/obarray.el
new file mode 100644
index 0000000..fb7a333
--- /dev/null
+++ b/lisp/obarray.el
@@ -0,0 +1,65 @@
+;;; obarray.el --- obarray functions -*- lexical-binding: t -*-
+
+;; Copyright (C) 2015 Free Software Foundation, Inc.
+
+;; Maintainer: address@hidden
+;; Keywords: obarray functions
+;; Package: emacs
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; This file provides function for working with obarrays.
+
+;;; Code:
+
+(defconst obarray-default-size 59
+  "The value 59 is an arbitrary prime number that gives a good hash.")
+
+(defun obarray-make (&optional size)
+  "Return a new obarray of size SIZE or `obarray-default-size'."
+  (let ((size (or size obarray-default-size)))
+    (if (< 0 size)
+        (make-vector size 0)
+      (signal 'wrong-type-argument '(size 0)))))
+
+(defun obarray-p (object)
+  "Return t if OBJECT is an obarray."
+  (and (vectorp object)
+       (< 0 (length object))))
+
+(defun obarray-get (obarray name)
+  "Return symbol named NAME if it is contained in OBARRAY.
+Return nil otherwise."
+  (intern-soft name obarray))
+
+(defun obarray-put (obarray name)
+  "Return symbol named NAME from OBARRAY.
+Creates and adds the symbol if doesn't exist."
+  (intern name obarray))
+
+(defun obarray-remove (obarray name)
+  "Remove symbol named NAME if it is contained in OBARRAY.
+Return t on success, nil otherwise."
+  (unintern name obarray))
+
+(defun obarray-foreach (fn obarray)
+  "Call function FN on every symbol in OBARRAY and return nil."
+  (mapatoms fn obarray))
+
+(provide 'obarray)
+;;; obarray.el ends here
diff --git a/test/automated/obarray-tests.el b/test/automated/obarray-tests.el
new file mode 100644
index 0000000..16ed694
--- /dev/null
+++ b/test/automated/obarray-tests.el
@@ -0,0 +1,90 @@
+;;; obarray-tests.el --- Tests for obarray -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2015 Free Software Foundation, Inc.
+
+;; Author: Przemysław Wojnowski <address@hidden>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'obarray)
+(require 'ert)
+
+(ert-deftest obarray-p-test ()
+  "Should assert that given object is an obarray."
+  (should-not (obarray-p 42))
+  (should-not (obarray-p "aoeu"))
+  (should-not (obarray-p '()))
+  (should-not (obarray-p []))
+  (should (obarray-p (make-vector 7 0))))
+
+(ert-deftest obarray-p-unchecked-content-test ()
+  "Should fail to check content of passed obarray."
+  :expected-result :failed
+  (should-not (obarray-p ["a" "b" "c"]))
+  (should-not (obarray-p [1 2 3])))
+
+(ert-deftest obarray-make-default-test ()
+  (let ((table (obarray-make)))
+    (should (obarray-p table))
+    (should (equal (make-vector 59 0) table))))
+
+(ert-deftest obarray-make-with-size-test ()
+  (should-error (obarray-make -1) :type 'wrong-type-argument)
+  (should-error (obarray-make 0) :type 'wrong-type-argument)
+  (let ((table (obarray-make 1)))
+    (should (obarray-p table))
+    (should (equal (make-vector 1 0) table))))
+
+(ert-deftest obarray-get-test ()
+  (let ((table (obarray-make 3)))
+    (should-not (obarray-get table "aoeu"))
+    (intern "aoeu" table)
+    (should (string= "aoeu" (obarray-get table "aoeu")))))
+
+(ert-deftest obarray-put-test ()
+  (let ((table (obarray-make 3)))
+    (should-not (obarray-get table "aoeu"))
+    (should (string= "aoeu" (obarray-put table "aoeu")))
+    (should (string= "aoeu" (obarray-get table "aoeu")))))
+
+(ert-deftest obarray-remove-test ()
+  (let ((table (obarray-make 3)))
+    (should-not (obarray-get table "aoeu"))
+    (should-not (obarray-remove table "aoeu"))
+    (should (string= "aoeu" (obarray-put table "aoeu")))
+    (should (string= "aoeu" (obarray-get table "aoeu")))
+    (should (obarray-remove table "aoeu"))
+    (should-not (obarray-get table "aoeu"))))
+
+(ert-deftest obarray-foreach-test ()
+  "Should execute function on all elements of obarray."
+  (let* ((table (obarray-make 3))
+         (syms '())
+         (collect-names (lambda (sym) (push (symbol-name sym) syms))))
+    (obarray-foreach collect-names table)
+    (should (null syms))
+    (obarray-put table "a")
+    (obarray-put table "b")
+    (obarray-put table "c")
+    (obarray-foreach collect-names table)
+    (should (equal (sort syms #'string<) '("a" "b" "c")))))
+
+(provide 'obarray-tests)
+;;; obarray-tests.el ends here
-- 
2.1.4

>From 9de31a0b27a8ea21ab072b825086d0fb97a5bc35 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Przemys=C5=82aw=20Wojnowski?= <address@hidden>
Date: Sun, 8 Nov 2015 19:19:15 +0100
Subject: [PATCH 2/2] Use obarray functions from obarray.

* lisp/abbrev.el (copy-abbrev-table, abbrev-table-p, make-abbrev-table,
  abbrev-table-get, abbrev-table-put, abbrev-table-empty-p,
  clear-abbrev-table, define-abbrev, abbrev--symbol, abbrev-table-menu):
  delegate to obarray.el functions.
* lisp/loadup.el: load obarray before abbrev
* test/automated/abbrev-tests.el: new tests
---
 lisp/abbrev.el                 | 50 +++++++++++++++++++++---------------------
 lisp/loadup.el                 |  1 +
 test/automated/abbrev-tests.el | 35 +++++++++++++++++++++++++++--
 3 files changed, 59 insertions(+), 27 deletions(-)

diff --git a/lisp/abbrev.el b/lisp/abbrev.el
index 43a905b..58ea7e4 100644
--- a/lisp/abbrev.el
+++ b/lisp/abbrev.el
@@ -33,6 +33,7 @@
 ;;; Code:
 
 (eval-when-compile (require 'cl-lib))
+(require 'obarray)
 
 (defgroup abbrev-mode nil
   "Word abbreviations mode."
@@ -87,7 +88,7 @@ be replaced by its expansion."
   "Make a new abbrev-table with the same abbrevs as TABLE.
 Does not copy property lists."
   (let ((new-table (make-abbrev-table)))
-    (mapatoms
+    (obarray-foreach
      (lambda (symbol)
        (define-abbrev new-table
         (symbol-name symbol)
@@ -406,12 +407,12 @@ A prefix argument means don't query; expand all abbrevs."
 
 (defun abbrev-table-get (table prop)
   "Get the PROP property of abbrev table TABLE."
-  (let ((sym (intern-soft "" table)))
+  (let ((sym (obarray-get table "")))
     (if sym (get sym prop))))
 
 (defun abbrev-table-put (table prop val)
   "Set the PROP property of abbrev table TABLE to VAL."
-  (let ((sym (intern "" table)))
+  (let ((sym (obarray-put table "")))
     (set sym nil)           ; Make sure it won't be confused for an abbrev.
     (put sym prop val)))
 
@@ -435,8 +436,7 @@ See `define-abbrev' for the effect of some special 
properties.
 (defun make-abbrev-table (&optional props)
   "Create a new, empty abbrev table object.
 PROPS is a list of properties."
-  ;; The value 59 is an arbitrary prime number.
-  (let ((table (make-vector 59 0)))
+  (let ((table (obarray-make)))
     ;; Each abbrev-table has a `modiff' counter which can be used to detect
     ;; when an abbreviation was added.  An example of use would be to
     ;; construct :regexp dynamically as the union of all abbrev names, so
@@ -451,7 +451,7 @@ PROPS is a list of properties."
 
 (defun abbrev-table-p (object)
   "Return non-nil if OBJECT is an abbrev table."
-  (and (vectorp object)
+  (and (obarray-p object)
        (numberp (abbrev-table-get object :abbrev-table-modiff))))
 
 (defun abbrev-table-empty-p (object &optional ignore-system)
@@ -460,12 +460,12 @@ If IGNORE-SYSTEM is non-nil, system definitions are 
ignored."
   (unless (abbrev-table-p object)
     (error "Non abbrev table object"))
   (not (catch 'some
-        (mapatoms (lambda (abbrev)
-                    (unless (or (zerop (length (symbol-name abbrev)))
-                                (and ignore-system
-                                     (abbrev-get abbrev :system)))
-                      (throw 'some t)))
-                  object))))
+        (obarray-foreach (lambda (abbrev)
+                            (unless (or (zerop (length (symbol-name abbrev)))
+                                        (and ignore-system
+                                             (abbrev-get abbrev :system)))
+                              (throw 'some t)))
+                          object))))
 
 (defvar global-abbrev-table (make-abbrev-table)
   "The abbrev table whose abbrevs affect all buffers.
@@ -529,12 +529,12 @@ the current abbrev table before abbrev lookup happens."
 (defun clear-abbrev-table (table)
   "Undefine all abbrevs in abbrev table TABLE, leaving it empty."
   (setq abbrevs-changed t)
-  (let* ((sym (intern-soft "" table)))
+  (let* ((sym (obarray-get table "")))
     (dotimes (i (length table))
       (aset table i 0))
     ;; Preserve the table's properties.
     (cl-assert sym)
-    (let ((newsym (intern "" table)))
+    (let ((newsym (obarray-put table "")))
       (set newsym nil)      ; Make sure it won't be confused for an abbrev.
       (setplist newsym (symbol-plist sym)))
     (abbrev-table-put table :abbrev-table-modiff
@@ -583,7 +583,7 @@ An obsolete but still supported calling form is:
   (setq props (plist-put props :abbrev-table-modiff
                          (abbrev-table-get table :abbrev-table-modiff)))
   (let ((system-flag (plist-get props :system))
-        (sym (intern name table)))
+        (sym (obarray-put table name)))
     ;; Don't override a prior user-defined abbrev with a system abbrev,
     ;; unless system-flag is `force'.
     (unless (and (not (memq system-flag '(nil force)))
@@ -673,10 +673,10 @@ The value is nil if that abbrev is not defined."
          ;; abbrevs do, we have to be careful.
          (sym
           ;; First try without case-folding.
-          (or (intern-soft abbrev table)
+          (or (obarray-get table abbrev)
               (when case-fold
                 ;; We didn't find any abbrev, try case-folding.
-                (let ((sym (intern-soft (downcase abbrev) table)))
+                (let ((sym (obarray-get table (downcase abbrev))))
                   ;; Only use it if it doesn't require :case-fixed.
                   (and sym (not (abbrev-get sym :case-fixed))
                        sym))))))
@@ -1005,14 +1005,14 @@ PROMPT is the prompt to use for the keymap.
 SORTFUN is passed to `sort' to change the default ordering."
   (unless sortfun (setq sortfun 'string-lessp))
   (let ((entries ()))
-    (mapatoms (lambda (abbrev)
-                (when (symbol-value abbrev)
-                  (let ((name (symbol-name abbrev)))
-                    (push `(,(intern name) menu-item ,name
-                            (lambda () (interactive)
-                              (abbrev-insert ',abbrev)))
-                          entries))))
-              table)
+    (obarray-foreach (lambda (abbrev)
+                       (when (symbol-value abbrev)
+                         (let ((name (symbol-name abbrev)))
+                           (push `(,(intern name) menu-item ,name
+                                   (lambda () (interactive)
+                                     (abbrev-insert ',abbrev)))
+                                 entries))))
+                     table)
     (nconc (make-sparse-keymap prompt)
            (sort entries (lambda (x y)
                 (funcall sortfun (nth 2 x) (nth 2 y)))))))
diff --git a/lisp/loadup.el b/lisp/loadup.el
index fef111f..15ecbc5 100644
--- a/lisp/loadup.el
+++ b/lisp/loadup.el
@@ -149,6 +149,7 @@
 (load "emacs-lisp/nadvice")
 (load "emacs-lisp/cl-preloaded")
 (load "minibuffer")            ;After loaddefs, for define-minor-mode.
+(load "obarray")        ;abbrev.el is implemented in terms of obarrays.
 (load "abbrev")         ;lisp-mode.el and simple.el use define-abbrev-table.
 (load "simple")
 
diff --git a/test/automated/abbrev-tests.el b/test/automated/abbrev-tests.el
index d08e026..17aea5d 100644
--- a/test/automated/abbrev-tests.el
+++ b/test/automated/abbrev-tests.el
@@ -1,4 +1,4 @@
-;;; abbrev-tests.el --- Test suite for abbrevs.
+;;; abbrev-tests.el --- Test suite for abbrevs  -*- lexical-binding: t; -*-
 
 ;; Copyright (C) 2015 Free Software Foundation, Inc.
 
@@ -20,11 +20,43 @@
 ;; You should have received a copy of the GNU General Public License
 ;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
 
+;;; Commentary:
+
 ;;; Code:
 
 (require 'ert)
 (require 'abbrev)
 
+(ert-deftest abbrev-table-p-test ()
+  (should-not (abbrev-table-p 42))
+  (should-not (abbrev-table-p "aoeu"))
+  (should-not (abbrev-table-p '()))
+  (should-not (abbrev-table-p []))
+  ;; Missing :abbrev-table-modiff counter:
+  (should-not (abbrev-table-p (obarray-make)))
+  (let* ((table (obarray-make)))
+    (abbrev-table-put table :abbrev-table-modiff 42)
+    (should (abbrev-table-p table))))
+
+(ert-deftest abbrev-make-abbrev-table-test ()
+  ;; Table without properties:
+  (let ((table (make-abbrev-table)))
+    (should (abbrev-table-p table))
+    (should (= (length table) obarray-default-size)))
+  ;; Table with one property 'foo with value 'bar:
+  (let ((table (make-abbrev-table '(foo bar))))
+    (should (abbrev-table-p table))
+    (should (= (length table) obarray-default-size))
+    (should (eq (abbrev-table-get table 'foo) 'bar))))
+
+(ert-deftest abbrev-table-get-put-test ()
+  (let ((table (make-abbrev-table)))
+    (should-not (abbrev-table-get table 'foo))
+    (should (= (abbrev-table-put table 'foo 42) 42))
+    (should (= (abbrev-table-get table 'foo) 42))
+    (should (eq (abbrev-table-put table 'foo 'bar) 'bar))
+    (should (eq (abbrev-table-get table 'foo) 'bar))))
+
 (ert-deftest copy-abbrev-table-test ()
   (defvar foo-abbrev-table nil)         ; Avoid compiler warning
   (define-abbrev-table 'foo-abbrev-table
@@ -39,5 +71,4 @@
   (should-not (string-equal (buffer-name) "*Backtrace*")))
 
 (provide 'abbrev-tests)
-
 ;;; abbrev-tests.el ends here
-- 
2.1.4