Re: Separating obarray handling from abbrevs

["Przemysław Wojnowski" <esperanto@cumego.com>]

[-- Attachment #1: Type: text/plain, Size: 827 bytes --]

"John Wiegley" <johnw@newartisans.com> writes:
>>>>>> Przemysław Wojnowski <esperanto@cumego.com> writes:
>
>> This way or another obarray's internals should not be abbrev's
>> concern. It should just use its API to implement own functionality. This
>> also ease testing, because both parts can be tested separately. Also
>> obarray functions could be reused in other places - quick grep shows
>> that obarray-make would be common.
>
> I like these changes, and agree that obarrays should have their own
> abstraction.
>
> John

I've extracted basic obarray functionality from abbrev.el into separate
file (the first patch).

The second patch makes abbrev.el to delegate to obarray-lib.

I don't understand one thing, what is purpose of "(intern "" obarray)".
If anyone could explain it would be great.


[-- Attachment #2: obarray-lib --]
[-- Type: text/x-diff, Size: 6546 bytes --]

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

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

diff --git a/lisp/obarray-lib.el b/lisp/obarray-lib.el
new file mode 100644
index 0000000..5ecb203
--- /dev/null
+++ b/lisp/obarray-lib.el
@@ -0,0 +1,64 @@
+;;; obarray-lib.el --- obarray functions -*- lexical-binding: t -*-
+
+;; Copyright (C) 2015 Free Software Foundation, Inc.
+
+;; Maintainer: emacs-devel@gnu.org
+;; 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 from obarray `OBARRAY' symbol with `NAME' or nil."
+  (intern-soft name obarray))
+
+(defun obarray-put (obarray name)
+  "Return form `OBARRAY' symbol with `NAME' or nil.
+Creates and adds the symbol if doesn't exist."
+  (intern name obarray))
+
+(defun obarray-remove (obarray name)
+  "Remove form `OBARRAY' symbol with `NAME'.
+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-lib)
+;;; obarray-lib.el ends here
diff --git a/test/automated/obarray-lib-tests.el b/test/automated/obarray-lib-tests.el
new file mode 100644
index 0000000..b9e4829
--- /dev/null
+++ b/test/automated/obarray-lib-tests.el
@@ -0,0 +1,90 @@
+;;; obarray-lib-tests.el --- Tests for obarray-lib -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2015 Free Software Foundation, Inc.
+
+;; Author: Przemysław Wojnowski <esperanto@cumego.com>
+
+;; 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-lib)
+(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-lib-tests)
+;;; obarray-lib-tests.el ends here
-- 
2.1.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: use obarray-lib in abbrev --]
[-- Type: text/x-diff, Size: 9140 bytes --]

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

* 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-lib functions.
* lisp/loadup.el: load obarray-lib 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..6dfeb10 100644
--- a/lisp/abbrev.el
+++ b/lisp/abbrev.el
@@ -33,6 +33,7 @@
 ;;; Code:
 
 (eval-when-compile (require 'cl-lib))
+(require 'obarray-lib)
 
 (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..e9d03d9 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-lib")    ;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