Separating obarray handling from abbrevs

Separating obarray handling from abbrevs

[Przemysław Wojnowski]

Hello everybody,

I'm trying to write a test for abbrev-table-p, but encountered a
problem. The function checks that passed object is a vector, but abbrev
tables are implemented in terms of obarrays, so in the following test,
the last two assertions fail:

(ert-deftest abbrev-table-p-test ()
  "Should assert that given object is an abbrev table."
  (should-not (abbrev-table-p 42))
  (should-not (abbrev-table-p "aoeu"))
  (should-not (abbrev-table-p '()))
  (should-not (abbrev-table-p []))
  (should-not (abbrev-table-p ["a" "b" "c"])))

The check in abbrev-table-p should have been "(obarray-p object)"
instead of "(vectorp object)". Of course, there's no obarray-p function,
but I can write it.

Another thing is that obarray "handling" is mixed with Abbrev
functionality in one file - they are separate concepts. obarray is just
a type of collection that is used as storage implementation and should
be clearly separated.

Can I move obarrays functionality from abbrev.el to a separate file and
changed abbrev.el to use it? Also this would allow to reuse that
functionality in other places where obarrays are used.

Thanks,
Przemysław

Re: Separating obarray handling from abbrevs

[Artur Malabarba]

2015-10-31 13:21 GMT+00:00 Przemysław Wojnowski <esperanto@cumego.com>:
> Can I move obarrays functionality from abbrev.el to a separate file and
> changed abbrev.el to use it? Also this would allow to reuse that
> functionality in other places where obarrays are used.

In principle I like that. But I had a quick look at abbrev.el and I
couldn't find the code you're referring to.
Exactly what functionality would you like to move?

Re: Separating obarray handling from abbrevs

[Przemysław Wojnowski]

Artur Malabarba <bruce.connor.am@gmail.com> writes:
> 2015-10-31 13:21 GMT+00:00 Przemysław Wojnowski <esperanto@cumego.com>:
>> Can I move obarrays functionality from abbrev.el to a separate file and
>> changed abbrev.el to use it? Also this would allow to reuse that
>> functionality in other places where obarrays are used.
>
> In principle I like that. But I had a quick look at abbrev.el and I
> couldn't find the code you're referring to.
> Exactly what functionality would you like to move?

For example creation of obarray from make-abbrev-table could be
extracted to separate method. So:
(let ((table (make-vector (or size 59) 0)))

would turn into:
;; in obarray-lib.el:
(defun obarray-make (&optional size)
  "Return a new obarray of size `SIZE' (defaults to 59).
The value 59 is an arbitrary prime number."
  (make-vector (or size 59) 0))

;; in abbrev.el/make-abbrev-table:
(let ((table (obarray-make)))
-------

Similarly:
(defun abbrev-table-p (object)
  "Return non-nil if OBJECT is an abbrev table."
  (and (vectorp object)
       (numberp (abbrev-table-get object :abbrev-table-modiff))))

into:
(defun obarray-p (object)
  "Return t if `OBJECT' is an obarray."
  (and (vectorp object)
       (< 0 (length object))
       ;; should check also table values?
       ))

(defun abbrev-table-p (object)
  "Return non-nil if OBJECT is an abbrev table."
  (and (obarray-p object)
       (numberp (abbrev-table-get object :abbrev-table-modiff))))
------

IMHO the code would be also a bit more readable with the following
functions:
;; not "map", because "map"s return values, this one not
(defun obarray-foreach (fn table)
  "Call function `FN' on every symbol in obarray `TABLE'."
  (mapatoms fn table))

(defun obarray-get (name table)
  "Return symbol with `NAME' from obarray `TABLE' or nil."
  (intern-soft name table))

(defun obarray-put (name table)
  "Return symbol with `NAME' form obarray `TABLE' or nil.
Create and add the symboly if doesn't exist."
  (intern name table))
------

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.

Re: Separating obarray handling from abbrevs

[John Wiegley]

>>>>> 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

Re: Separating obarray handling from abbrevs

[Przemysław Wojnowski]

[-- 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

Re: Separating obarray handling from abbrevs

[Eli Zaretskii]

> From: Przemysław Wojnowski <esperanto@cumego.com>
> Date: Sun, 08 Nov 2015 19:50:54 +0100
> Cc: Artur Malabarba <bruce.connor.am@gmail.com>,
> 	Emacs Developers <emacs-devel@gnu.org>
> 
> I don't understand one thing, what is purpose of "(intern "" obarray)".
> If anyone could explain it would be great.

Please tell what is unclear or confusing in that expression.

Re: Separating obarray handling from abbrevs

[Przemysław Wojnowski]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Przemysław Wojnowski <esperanto@cumego.com>
>> I don't understand one thing, what is purpose of "(intern "" obarray)".
>> If anyone could explain it would be great.
>
> Please tell what is unclear or confusing in that expression.

I think I understand now... abbrev is using "" to create a (unique)
symbol in which it will store its metadata as properties. "" presumably
has been chosen to not interfere with abbrevs that will go into the
same obarray. Am I correct?

Thanks!

Re: Separating obarray handling from abbrevs

[Eli Zaretskii]

> From: Przemysław Wojnowski <esperanto@cumego.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 08 Nov 2015 20:09:53 +0100
> 
> I think I understand now... abbrev is using "" to create a (unique)
> symbol in which it will store its metadata as properties. "" presumably
> has been chosen to not interfere with abbrevs that will go into the
> same obarray. Am I correct?

Yes, I think so.

Re: Separating obarray handling from abbrevs

[Artur Malabarba]

Przemysław Wojnowski <esperanto@cumego.com> writes:

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

I kinda failed to see the point of this originally, but looking at the
diff I can tell this is a good idea.™

> +(defun obarray-make (&optional size)
> +  "Return a new obarray of size `SIZE' or `obarray-default-size'."

Use SIZE, not `SIZE'. Similarly everywhere else.

> +(defun obarray-get (obarray name)
> +  "Return from obarray `OBARRAY' symbol with `NAME' or nil."

I'd use:
"Return symbol named NAME if it is contained in OBARRAY.
Return nil otherwise."

And I would repeat this throughout the docstrings.

> +(defun obarray-put (obarray name)
> +  "Return form `OBARRAY' symbol with `NAME' or nil.

Why the nil?

Re: Separating obarray handling from abbrevs

[Przemysław Wojnowski]

Artur Malabarba <bruce.connor.am@gmail.com> writes:

>> +(defun obarray-make (&optional size)
>> +  "Return a new obarray of size `SIZE' or `obarray-default-size'."
>
> Use SIZE, not `SIZE'. Similarly everywhere else.
Are you sure about that? With them comments look happier and
obarray-default-size becomes a link in Help buffer. Flycheck is also
happy.

>> +(defun obarray-get (obarray name)
>> +  "Return from obarray `OBARRAY' symbol with `NAME' or nil."
>
> I'd use:
> "Return symbol named NAME if it is contained in OBARRAY.
> Return nil otherwise."
>
> And I would repeat this throughout the docstrings.
Will do that.

>> +(defun obarray-put (obarray name)
>> +  "Return form `OBARRAY' symbol with `NAME' or nil.
>
> Why the nil?
It's a bug. I was just too fast and too furious when writing this. ;-)

Re: Separating obarray handling from abbrevs

[Stephen Leake]

Przemysław Wojnowski <esperanto@cumego.com> writes:

> Artur Malabarba <bruce.connor.am@gmail.com> writes:
>
>>> +(defun obarray-make (&optional size)
>>> +  "Return a new obarray of size `SIZE' or `obarray-default-size'."
>>
>> Use SIZE, not `SIZE'. Similarly everywhere else.

The markup for arguments (like "size") is all caps. The markup for other
variables/defuns (like "obarray-default-size") is quotes.

>>> +(defun obarray-get (obarray name)
>>> +  "Return from obarray `OBARRAY' symbol with `NAME' or nil."
>>
>> I'd use:
>> "Return symbol named NAME if it is contained in OBARRAY.
>> Return nil otherwise."
>>
>> And I would repeat this throughout the docstrings.
> Will do that.

Then you have to ignore complaints from the comment style checker, which
insists on listing the arguments in order in the doc string. There was a
proposal recently to delete that check.

-- 
-- Stephe

Re: Separating obarray handling from abbrevs

[Przemysław Wojnowski]

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

Artur Malabarba <bruce.connor.am@gmail.com> writes:
>> +(defun obarray-make (&optional size)
>> +  "Return a new obarray of size `SIZE' or `obarray-default-size'."
>
> Use SIZE, not `SIZE'. Similarly everywhere else.
Done.

>> +(defun obarray-get (obarray name)
>> +  "Return from obarray `OBARRAY' symbol with `NAME' or nil."
>
> I'd use:
> "Return symbol named NAME if it is contained in OBARRAY.
> Return nil otherwise."
>
> And I would repeat this throughout the docstrings.
Done.

>> +(defun obarray-put (obarray name)
>> +  "Return form `OBARRAY' symbol with `NAME' or nil.
>
> Why the nil?
Done.


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

From cff1d1bb745f6b9e890a0ca75460f6451e9067f1 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                 | 65 +++++++++++++++++++++++++++
 test/automated/obarray-lib-tests.el | 90 +++++++++++++++++++++++++++++++++++++
 2 files changed, 155 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..f80ba5a
--- /dev/null
+++ b/lisp/obarray-lib.el
@@ -0,0 +1,65 @@
+;;; 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 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 form 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-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 d8636fdd0344d9e64f02744c86039f2a98cce28c 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

Re: Separating obarray handling from abbrevs

[Artur Malabarba]

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

> Done.

There's also a typo where you write form instead of from.
Other than that, LGTM.

[-- Attachment #2: Type: text/html, Size: 136 bytes --]

Re: Separating obarray handling from abbrevs

[Przemysław Wojnowski]

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

Artur Malabarba <bruce.connor.am@gmail.com> writes:

> There's also a typo where you write form instead of from.
Fixed.


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

From d0e90f253ea3fefbb024ae512891e2a2c8f8d3d2 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                 | 65 +++++++++++++++++++++++++++
 test/automated/obarray-lib-tests.el | 90 +++++++++++++++++++++++++++++++++++++
 2 files changed, 155 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..be6007e
--- /dev/null
+++ b/lisp/obarray-lib.el
@@ -0,0 +1,65 @@
+;;; 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 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-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 b78f84205102a710b4c339ee8958e3f04c20adff 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

Re: Separating obarray handling from abbrevs

[Nicolas Petton]

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

Przemysław Wojnowski <esperanto@cumego.com> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: Separating obarray handling from abbrevs

[John Wiegley]

>>>>> Nicolas Petton <nicolas@petton.fr> writes:

> 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.

I too would prefer obarray.el.

John

Re: Separating obarray handling from abbrevs

[Przemysław Wojnowski]

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

Nicolas Petton <nicolas@petton.fr> writes:

> Przemysław Wojnowski <esperanto@cumego.com> 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.


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

From 2dabe20527a3b53d345006648cca89522f4eff3e 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.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: 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 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 <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)
+(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


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

From 9de31a0b27a8ea21ab072b825086d0fb97a5bc35 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.

* 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

Re: Separating obarray handling from abbrevs

[John Wiegley]

>>>>> Przemysław Wojnowski <esperanto@cumego.com> writes:

> Here are corrected versions.

I like it. I'm not a big fan of the name "obarray-foreach", since we're just
choosing a different name for "map", without much existing precedent for doing
so. I'd prefer "obarray-map".

Other than that, let's apply and push. Nicolas, would you do the honors? We
can adjust the naming of obarray-foreach after the push, unless others do like
that name.

John

Re: Separating obarray handling from abbrevs

[Eli Zaretskii]

> From: John Wiegley <jwiegley@gmail.com>
> Date: Tue, 10 Nov 2015 12:32:16 -0800
> Cc: Nicolas Petton <nicolas@petton.fr>,
> 	Artur Malabarba <bruce.connor.am@gmail.com>,
> 	emacs-devel <emacs-devel@gnu.org>
> 
> >>>>> Przemysław Wojnowski <esperanto@cumego.com> writes:
> 
> > Here are corrected versions.
> 
> I like it. I'm not a big fan of the name "obarray-foreach", since we're just
> choosing a different name for "map", without much existing precedent for doing
> so. I'd prefer "obarray-map".

Name bikeshedding? yay!

We have map-char-table and map-keymap, so I'd suggest map-obarray for
consistency.

Re: Separating obarray handling from abbrevs

[Przemysław Wojnowski]

Eli Zaretskii <eliz@gnu.org> writes:

JohnW> I'd prefer "obarray-map".

> We have map-char-table and map-keymap, so I'd suggest map-obarray for
> consistency.

FWIW I would also prefer "obarray-map" for the same reason, but to be
consistent with other obarray functions - all would start consistently
with "obarray-", which would make them easily "completable" and
discoverable.

Re: Separating obarray handling from abbrevs

[Ken Raeburn]

> On Nov 10, 2015, at 15:54, Przemysław Wojnowski <esperanto@cumego.com> wrote:
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> JohnW> I'd prefer "obarray-map".
> 
>> We have map-char-table and map-keymap, so I'd suggest map-obarray for
>> consistency.
> 
> FWIW I would also prefer "obarray-map" for the same reason, but to be
> consistent with other obarray functions - all would start consistently
> with "obarray-", which would make them easily "completable" and
> discoverable.

It seems to be common practice, though, for object creation functions to be named “make-THING” (or “PACKAGE-make-THING”) rather than “THING-make”.  So, “make-obarray” is probably preferable to “obarray-make”.

Ken

Re: Separating obarray handling from abbrevs

[Nicolas Petton]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Name bikeshedding? yay!
>
> We have map-char-table and map-keymap, so I'd suggest map-obarray for
> consistency.

That would not follow the name prefix, so I'd name it `obarray-map'
instead.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: Separating obarray handling from abbrevs

[Przemysław Wojnowski]

John Wiegley <jwiegley@gmail.com> writes:
> I like it. I'm not a big fan of the name "obarray-foreach", since we're just
> choosing a different name for "map", without much existing precedent for doing
> so. I'd prefer "obarray-map".
I was thinking about "map" suffix, but decided not to used it, because
"map" functions return something meaningful and this one only nil. :-|

Re: Separating obarray handling from abbrevs

[Artur Malabarba]

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

On 10 Nov 2015 8:39 pm, "Przemysław Wojnowski" <esperanto@cumego.com> wrote:
>
> John Wiegley <jwiegley@gmail.com> writes:
> > I like it. I'm not a big fan of the name "obarray-foreach", since we're
just
> > choosing a different name for "map", without much existing precedent
for doing
> > so. I'd prefer "obarray-map".
> I was thinking about "map" suffix, but decided not to used it, because
> "map" functions return something meaningful and this one only nil. :-|

Then call it mapc. ;-)

[-- Attachment #2: Type: text/html, Size: 764 bytes --]

RE: Separating obarray handling from abbrevs

[Drew Adams]

> > > I'd prefer "obarray-map".
> > I was thinking about "map" suffix, but decided not to used it, because
> > "map" functions return something meaningful and this one only nil. :-|
> Then call it mapc. ;-)

They don't all return something useful - see `map-keymap', for instance.

Re: Separating obarray handling from abbrevs

[John Wiegley]

>>>>> Artur Malabarba <bruce.connor.am@gmail.com> writes:

>> I was thinking about "map" suffix, but decided not to used it, because
>> "map" functions return something meaningful and this one only nil. :-|

> Then call it mapc. ;-)

OK, this is grey enough of an area, obarray-foreach is fine. Whichever the
author prefers.

John

Re: Separating obarray handling from abbrevs

[Eli Zaretskii]

> From: Przemysław Wojnowski <esperanto@cumego.com>
> Date: Tue, 10 Nov 2015 21:39:25 +0100
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> I was thinking about "map" suffix, but decided not to used it, because
> "map" functions return something meaningful and this one only nil. :-|

mapc doesn't return anything.

RE: Separating obarray handling from abbrevs

[Drew Adams]

> > I was thinking about "map" suffix, but decided not to used it, because
> > "map" functions return something meaningful and this one only nil. :-|
> 
> mapc doesn't return anything.

It returns the input sequence - it is even documented as doing so.

Maybe you meant that it doesn't return anything else that might
be particularly interesting.

(`map-keymap' is a "map" function whose return value is not
documented.)

Re: Separating obarray handling from abbrevs

[David Kastrup]

Przemysław Wojnowski <esperanto@cumego.com> writes:

> John Wiegley <jwiegley@gmail.com> writes:
>> I like it. I'm not a big fan of the name "obarray-foreach", since we're just
>> choosing a different name for "map", without much existing precedent for doing
>> so. I'd prefer "obarray-map".
> I was thinking about "map" suffix, but decided not to used it, because
> "map" functions return something meaningful and this one only nil. :-|

mapc returns nothing meaningful (the original sequence) as opposed to
mapcar.  You could return the original obarray.  I'm not sure that's
much more useful than nil, however.

-- 
David Kastrup

Re: Separating obarray handling from abbrevs

[Nicolas Petton]

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

John Wiegley <jwiegley@gmail.com> writes:

> I like it. I'm not a big fan of the name "obarray-foreach", since we're just
> choosing a different name for "map", without much existing precedent for doing
> so. I'd prefer "obarray-map".

Agreed.

>
> Other than that, let's apply and push. Nicolas, would you do the
> honors?

Sure.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: Separating obarray handling from abbrevs

[Nicolas Petton]

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

John Wiegley <jwiegley@gmail.com> writes:
> Other than that, let's apply and push.

I just pushed it to master.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: Separating obarray handling from abbrevs

[John Wiegley]

>>>>> Nicolas Petton <nicolas@petton.fr> writes:

> John Wiegley <jwiegley@gmail.com> writes:
>> Other than that, let's apply and push.

> I just pushed it to master.

Thank you, Nico!

John

Re: Separating obarray handling from abbrevs

[Nicolas Petton]

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

R

[-- Attachment #2.1: Type: text/plain, Size: 313 bytes --]

John Wiegley <jwiegley@gmail.com> writes:

>> I just pushed it to master.

I forgot the second patch.  It's now also pushed.

One question: I was offline the last few days, and I missed the emacs-25
branch discussion.  I guess I should not push this to emacs-25 as we are
already in feature freeze?

Cheers,
Nico

[-- Attachment #2.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: Separating obarray handling from abbrevs

[John Wiegley]

>>>>> Nicolas Petton <nicolas@petton.fr> writes:

> One question: I was offline the last few days, and I missed the emacs-25
> branch discussion. I guess I should not push this to emacs-25 as we are
> already in feature freeze?

You are right, it should go to master, since it represents a new API with new
accompanying documentation.

John

Re: Separating obarray handling from abbrevs

[Artur Malabarba]

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

I didn't quite get what this "second patch" is. Is it just fixes for
obarray.el or something new?

2015-11-20 0:13 GMT+00:00 John Wiegley <jwiegley@gmail.com>:

> >>>>> Nicolas Petton <nicolas@petton.fr> writes:
>
> > One question: I was offline the last few days, and I missed the emacs-25
> > branch discussion. I guess I should not push this to emacs-25 as we are
> > already in feature freeze?
>
> You are right, it should go to master, since it represents a new API with
> new
> accompanying documentation.
>
> John
>

[-- Attachment #2: Type: text/html, Size: 996 bytes --]

Re: Separating obarray handling from abbrevs

[Przemysław Wojnowski]

The second patch doesn't fix obarry.el. It just makes abbrev.el to use
functions from obarray.el - this was the whole point of this separation.

W dniu 20.11.2015 o 02:01, Artur Malabarba pisze:
> I didn't quite get what this "second patch" is. Is it just fixes for obarray.el
> or something new?
>
> 2015-11-20 0:13 GMT+00:00 John Wiegley <jwiegley@gmail.com
> <mailto:jwiegley@gmail.com>>:
>
>      >>>>> Nicolas Petton <nicolas@petton.fr <mailto:nicolas@petton.fr>> writes:
>
>      > One question: I was offline the last few days, and I missed the emacs-25
>      > branch discussion. I guess I should not push this to emacs-25 as we are
>      > already in feature freeze?
>
>     You are right, it should go to master, since it represents a new API with new
>     accompanying documentation.
>
>     John
>
>

Re: Separating obarray handling from abbrevs

[Nicolas Petton]

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

Przemysław Wojnowski <esperanto@cumego.com> writes:

> Thanks for the feedback!

You're welcome!

> +(defun obarray-p (object)

Could be `obarrayp', since it's one word, like `listp'.

Note to myself: In the future, I would really like map.el to be
extensible the way seq.el is.  This way, obarray.el could implement the
`map' protocol, and reuse all the functions defined in map.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]