From: Paul Eggert <eggert@cs.ucla.edu>
To: 12215@debbugs.gnu.org
Cc: Dmitry Antipov <dmantipov@yandex.ru>
Subject: bug#12215: CSET is unnecessarily confusing
Date: Thu, 16 Aug 2012 17:04:37 -0700 [thread overview]
Message-ID: <502D8A95.7060609@cs.ucla.edu> (raw)
Recent changes to Emacs have introduced code like the following:
CSET (XCHAR_TABLE (char_table), parent, parent);
This is unnecessarily confusing. Those two 'parent' expressions
refer to different things; the first 'parent' is not really a C
expression at all. I recall that Stefan also expressed unease about
CSET's not acting like a function, in this respect.
It's easy to change lisp.h so that the same code can be
written as follows, which is shorter and clearer:
char_table_set_parent (char_table, parent);
The main objection to changing lisp.h, if I recall correctly, is that
it will make lisp.h longer, since lisp.h will need a separate setter
function for each field. But that's not much of a problem since
these functions are really simple. And the advantage of readability
in users of the code makes the .h change worthwhile.
Here's a patch to make this change for CSET. I'd like to install this,
along with similar patches for the other non-function ?SET macros defined
recently.
=== modified file 'src/ChangeLog'
--- src/ChangeLog 2012-08-16 21:58:44 +0000
+++ src/ChangeLog 2012-08-16 23:59:55 +0000
@@ -1,5 +1,14 @@
2012-08-16 Paul Eggert <eggert@cs.ucla.edu>
+ * lisp.h (CSET): Remove.
+ (char_table_set_ascii, char_table_set_defalt, char_table_set_parent)
+ (char_table_set_purpose): New functions,
+ replacing CSET. All uses changed. For example, replace
+ "CSET (XCHAR_TABLE (char_table), parent, parent);" with
+ "char_table_set_parent (char_table, parent);".
+ The old version was confusing because it used the same name
+ 'parent' for two different things.
+
Use ASCII tests for character types.
* category.c, dispnew.c, doprnt.c, editfns.c, syntax.c, term.c:
* xfns.c, xterm.c:
=== modified file 'src/casetab.c'
--- src/casetab.c 2012-08-16 03:13:44 +0000
+++ src/casetab.c 2012-08-16 23:59:55 +0000
@@ -260,7 +260,7 @@
down = Fmake_char_table (Qcase_table, Qnil);
Vascii_downcase_table = down;
- CSET (XCHAR_TABLE (down), purpose, Qcase_table);
+ char_table_set_purpose (down, Qcase_table);
for (i = 0; i < 128; i++)
{
=== modified file 'src/category.c'
--- src/category.c 2012-08-16 21:58:44 +0000
+++ src/category.c 2012-08-16 23:59:55 +0000
@@ -238,8 +238,8 @@
table = copy_char_table (table);
if (! NILP (XCHAR_TABLE (table)->defalt))
- CSET (XCHAR_TABLE (table), defalt,
- Fcopy_sequence (XCHAR_TABLE (table)->defalt));
+ char_table_set_defalt (table,
+ Fcopy_sequence (XCHAR_TABLE (table)->defalt));
char_table_set_extras
(table, 0, Fcopy_sequence (XCHAR_TABLE (table)->extras[0]));
map_char_table (copy_category_entry, Qnil, table, table);
@@ -270,7 +270,7 @@
int i;
val = Fmake_char_table (Qcategory_table, Qnil);
- CSET (XCHAR_TABLE (val), defalt, MAKE_CATEGORY_SET);
+ char_table_set_defalt (val, MAKE_CATEGORY_SET);
for (i = 0; i < (1 << CHARTAB_SIZE_BITS_0); i++)
char_table_set_contents (val, i, MAKE_CATEGORY_SET);
Fset_char_table_extra_slot (val, make_number (0),
@@ -466,7 +466,7 @@
Vstandard_category_table = Fmake_char_table (Qcategory_table, Qnil);
/* Set a category set which contains nothing to the default. */
- CSET (XCHAR_TABLE (Vstandard_category_table), defalt, MAKE_CATEGORY_SET);
+ char_table_set_defalt (Vstandard_category_table, MAKE_CATEGORY_SET);
Fset_char_table_extra_slot (Vstandard_category_table, make_number (0),
Fmake_vector (make_number (95), Qnil));
}
=== modified file 'src/chartab.c'
--- src/chartab.c 2012-08-16 07:26:18 +0000
+++ src/chartab.c 2012-08-16 23:59:55 +0000
@@ -115,8 +115,8 @@
size = VECSIZE (struct Lisp_Char_Table) - 1 + n_extras;
vector = Fmake_vector (make_number (size), init);
XSETPVECTYPE (XVECTOR (vector), PVEC_CHAR_TABLE);
- CSET (XCHAR_TABLE (vector), parent, Qnil);
- CSET (XCHAR_TABLE (vector), purpose, purpose);
+ char_table_set_parent (vector, Qnil);
+ char_table_set_purpose (vector, purpose);
XSETCHAR_TABLE (vector, XCHAR_TABLE (vector));
return vector;
}
@@ -185,16 +185,16 @@
copy = Fmake_vector (make_number (size), Qnil);
XSETPVECTYPE (XVECTOR (copy), PVEC_CHAR_TABLE);
- CSET (XCHAR_TABLE (copy), defalt, XCHAR_TABLE (table)->defalt);
- CSET (XCHAR_TABLE (copy), parent, XCHAR_TABLE (table)->parent);
- CSET (XCHAR_TABLE (copy), purpose, XCHAR_TABLE (table)->purpose);
+ char_table_set_defalt (copy, XCHAR_TABLE (table)->defalt);
+ char_table_set_parent (copy, XCHAR_TABLE (table)->parent);
+ char_table_set_purpose (copy, XCHAR_TABLE (table)->purpose);
for (i = 0; i < chartab_size[0]; i++)
char_table_set_contents
- (copy, i,
+ (copy, i,
(SUB_CHAR_TABLE_P (XCHAR_TABLE (table)->contents[i])
? copy_sub_char_table (XCHAR_TABLE (table)->contents[i])
: XCHAR_TABLE (table)->contents[i]));
- CSET (XCHAR_TABLE (copy), ascii, char_table_ascii (copy));
+ char_table_set_ascii (copy, char_table_ascii (copy));
size -= VECSIZE (struct Lisp_Char_Table) - 1;
for (i = 0; i < size; i++)
char_table_set_extras (copy, i, XCHAR_TABLE (table)->extras[i]);
@@ -436,7 +436,7 @@
}
sub_char_table_set (sub, c, val, UNIPROP_TABLE_P (table));
if (ASCII_CHAR_P (c))
- CSET (tbl, ascii, char_table_ascii (table));
+ char_table_set_ascii (table, char_table_ascii (table));
}
return val;
}
@@ -512,7 +512,7 @@
}
}
if (ASCII_CHAR_P (from))
- CSET (tbl, ascii, char_table_ascii (table));
+ char_table_set_ascii (table, char_table_ascii (table));
}
return val;
}
@@ -562,7 +562,7 @@
error ("Attempt to make a chartable be its own parent");
}
- CSET (XCHAR_TABLE (char_table), parent, parent);
+ char_table_set_parent (char_table, parent);
return parent;
}
@@ -640,12 +640,12 @@
{
int i;
- CSET (XCHAR_TABLE (char_table), ascii, value);
+ char_table_set_ascii (char_table, value);
for (i = 0; i < chartab_size[0]; i++)
char_table_set_contents (char_table, i, value);
}
else if (EQ (range, Qnil))
- CSET (XCHAR_TABLE (char_table), defalt, value);
+ char_table_set_defalt (char_table, value);
else if (CHARACTERP (range))
char_table_set (char_table, XINT (range), value);
else if (CONSP (range))
@@ -736,7 +736,7 @@
(char_table, i, optimize_sub_char_table (elt, test));
}
/* Reset the `ascii' cache, in case it got optimized away. */
- CSET (XCHAR_TABLE (char_table), ascii, char_table_ascii (char_table));
+ char_table_set_ascii (char_table, char_table_ascii (char_table));
return Qnil;
}
@@ -828,9 +828,9 @@
/* This is to get a value of FROM in PARENT
without checking the parent of PARENT. */
- CSET (XCHAR_TABLE (parent), parent, Qnil);
+ char_table_set_parent (parent, Qnil);
val = CHAR_TABLE_REF (parent, from);
- CSET (XCHAR_TABLE (parent), parent, temp);
+ char_table_set_parent (parent, temp);
XSETCDR (range, make_number (c - 1));
val = map_sub_char_table (c_function, function,
parent, arg, val, range,
@@ -910,9 +910,9 @@
temp = XCHAR_TABLE (parent)->parent;
/* This is to get a value of FROM in PARENT without checking the
parent of PARENT. */
- CSET (XCHAR_TABLE (parent), parent, Qnil);
+ char_table_set_parent (parent, Qnil);
val = CHAR_TABLE_REF (parent, from);
- CSET (XCHAR_TABLE (parent), parent, temp);
+ char_table_set_parent (parent, temp);
val = map_sub_char_table (c_function, function, parent, arg, val, range,
parent);
table = parent;
@@ -1350,7 +1350,7 @@
: ! NILP (val))
return Qnil;
/* Prepare ASCII values in advance for CHAR_TABLE_REF. */
- CSET (XCHAR_TABLE (table), ascii, char_table_ascii (table));
+ char_table_set_ascii (table, char_table_ascii (table));
return table;
}
=== modified file 'src/fns.c'
--- src/fns.c 2012-08-16 03:13:44 +0000
+++ src/fns.c 2012-08-16 23:59:55 +0000
@@ -2151,7 +2151,7 @@
for (i = 0; i < (1 << CHARTAB_SIZE_BITS_0); i++)
char_table_set_contents (array, i, item);
- CSET (XCHAR_TABLE (array), defalt, item);
+ char_table_set_defalt (array, item);
}
else if (STRINGP (array))
{
=== modified file 'src/fontset.c'
--- src/fontset.c 2012-08-16 03:13:44 +0000
+++ src/fontset.c 2012-08-16 23:59:55 +0000
@@ -1979,7 +1979,7 @@
if (c <= MAX_5_BYTE_CHAR)
char_table_set_range (tables[k], c, to, alist);
else
- CSET (XCHAR_TABLE (tables[k]), defalt, alist);
+ char_table_set_defalt (tables[k], alist);
/* At last, change each elements to font names. */
for (; CONSP (alist); alist = XCDR (alist))
=== modified file 'src/lisp.h'
--- src/lisp.h 2012-08-16 07:58:24 +0000
+++ src/lisp.h 2012-08-16 23:59:55 +0000
@@ -936,12 +936,6 @@
extern const int chartab_size[4];
-/* Most code should use this macro to set non-array Lisp fields in struct
- Lisp_Char_Table. For CONTENTS and EXTRAS, use char_table_set_contents
- and char_table_set_extras, respectively. */
-
-#define CSET(c, field, value) ((c)->field = (value))
-
struct Lisp_Char_Table
{
/* HEADER.SIZE is the vector's size field, which also holds the
@@ -2439,6 +2433,30 @@
XSTRING (s)->intervals = i;
}
+/* Set a Lisp slot in TABLE to VAL. Most code should use this instead
+ of setting slots directly. */
+
+LISP_INLINE void
+char_table_set_ascii (Lisp_Object table, Lisp_Object val)
+{
+ XCHAR_TABLE (table)->ascii = val;
+}
+LISP_INLINE void
+char_table_set_defalt (Lisp_Object table, Lisp_Object val)
+{
+ XCHAR_TABLE (table)->defalt = val;
+}
+LISP_INLINE void
+char_table_set_parent (Lisp_Object table, Lisp_Object val)
+{
+ XCHAR_TABLE (table)->parent = val;
+}
+LISP_INLINE void
+char_table_set_purpose (Lisp_Object table, Lisp_Object val)
+{
+ XCHAR_TABLE (table)->purpose = val;
+}
+
/* Set different slots in (sub)character tables. */
LISP_INLINE void
=== modified file 'src/syntax.c'
--- src/syntax.c 2012-08-16 21:58:44 +0000
+++ src/syntax.c 2012-08-16 23:59:55 +0000
@@ -818,7 +818,7 @@
/* Only the standard syntax table should have a default element.
Other syntax tables should inherit from parents instead. */
- CSET (XCHAR_TABLE (copy), defalt, Qnil);
+ char_table_set_defalt (copy, Qnil);
/* Copied syntax tables should all have parents.
If we copied one with no parent, such as the standard syntax table,
next reply other threads:[~2012-08-17 0:04 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-08-17 0:04 Paul Eggert [this message]
2012-08-17 4:12 ` bug#12215: CSET is unnecessarily confusing Dmitry Antipov
2012-08-21 16:55 ` Stefan Monnier
2012-08-22 3:25 ` Paul Eggert
2012-08-22 13:27 ` Stefan Monnier
2012-08-22 16:35 ` Paul Eggert
2012-08-22 16:50 ` Dmitry Antipov
2012-08-23 7:02 ` Paul Eggert
2012-08-23 12:26 ` Stefan Monnier
2012-08-23 14:40 ` Paul Eggert
2012-08-24 3:46 ` Chong Yidong
2012-08-24 3:57 ` Paul Eggert
2012-08-24 4:26 ` Dmitry Antipov
2012-08-24 15:10 ` Stefan Monnier
2012-08-24 17:19 ` Paul Eggert
2012-08-24 17:27 ` Tom Tromey
2012-08-24 18:11 ` Paul Eggert
2012-08-24 21:12 ` Stefan Monnier
2012-08-25 0:17 ` Paul Eggert
2012-08-25 1:58 ` Stefan Monnier
2012-08-26 5:05 ` Paul Eggert
2012-08-21 17:55 ` Stefan Monnier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=502D8A95.7060609@cs.ucla.edu \
--to=eggert@cs.ucla.edu \
--cc=12215@debbugs.gnu.org \
--cc=dmantipov@yandex.ru \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.