Package: emacs;
Reported by: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Fri, 17 Aug 2012 00:14:01 UTC
Severity: normal
Tags: patch
Done: Paul Eggert <eggert <at> cs.ucla.edu>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: help-debbugs <at> gnu.org (GNU bug Tracking System) To: Paul Eggert <eggert <at> cs.ucla.edu> Cc: tracker <at> debbugs.gnu.org Subject: bug#12215: closed (CSET is unnecessarily confusing) Date: Sat, 18 Aug 2012 06:21:02 +0000
[Message part 1 (text/plain, inline)]
Your message dated Fri, 17 Aug 2012 23:20:54 -0700 with message-id <502F3446.8000907 <at> cs.ucla.edu> and subject line Fwd: Re: CSET is unnecessarily confusing has caused the debbugs.gnu.org bug report #12215, regarding CSET is unnecessarily confusing to be marked as done. (If you believe you have received this mail in error, please contact help-debbugs <at> gnu.org.) -- 12215: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=12215 GNU Bug Tracking System Contact help-debbugs <at> gnu.org with problems
[Message part 2 (message/rfc822, inline)]
From: Paul Eggert <eggert <at> cs.ucla.edu> To: bug-gnu-emacs <at> gnu.org Cc: Dmitry Antipov <dmantipov <at> yandex.ru> Subject: CSET is unnecessarily confusing Date: Thu, 16 Aug 2012 17:04:37 -0700Recent 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 <at> 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,
[Message part 3 (message/rfc822, inline)]
From: Paul Eggert <eggert <at> cs.ucla.edu> To: 12215-done <at> debbugs.gnu.org Subject: Fwd: Re: CSET is unnecessarily confusing Date: Fri, 17 Aug 2012 23:20:54 -0700Thanks, I installed changes to replace all uses of CSET, BSET, etc. with setter functions, and am marking this as done. I left BVAR and FVAR alone for now. I don't see what they're used for, though -- they're the same in the concurrency branch as in the trunk. And they sometimes are confusing, too, since they look like functions but cannot be implemented as functions.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.