bug#12215: CSET is unnecessarily confusing

bug#12215: CSET is unnecessarily confusing

[Paul Eggert]

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,

bug#12215: CSET is unnecessarily confusing

[Dmitry Antipov]

On 08/17/2012 04:04 AM, Paul Eggert wrote:
> 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.

OK

Dmitry

bug#12215: CSET is unnecessarily confusing

[Stefan Monnier]

> Recent changes to Emacs have introduced code like the following:
>    CSET (XCHAR_TABLE (char_table), parent, parent);
> This is unnecessarily confusing.

An alternative to the umpteen macros/functions is to use

     CSET (XCHAR_TABLE (char_table), ->parent, parent);


-- Stefan

bug#12215: CSET is unnecessarily confusing

[Stefan Monnier]

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

I already said I do not like this solution, so please wait before an
explicit go ahead before committing such changes.

Guys, there's no hurry here.  Learn to work on branches.


        Stefan "tired of all those back&forth changes all over the place
                resulting in endless repetitions of merge conflicts"


PS: Wow, I managed to avoid nasty expletives.

bug#12215: CSET is unnecessarily confusing

[Paul Eggert]

On 08/21/2012 09:55 AM, Stefan Monnier wrote:
> CSET (XCHAR_TABLE (char_table), ->parent, parent);

That does avoid the ambiguity but it's pretty weird.

How about something like this instead?

  cset (XCHAR_TABLE (char_table), cset_parent, parent);

where cset_parent is an offset into the structure, or
is a member of a simple enumeration, whichever seems
cleaner.  This would be functional and relatively
straightforward.

bug#12215: CSET is unnecessarily confusing

[Stefan Monnier]

>> CSET (XCHAR_TABLE (char_table), ->parent, parent);
> That does avoid the ambiguity but it's pretty weird.

Less weird than CSET (XCHAR_TABLE (char_table), parent, parent),
and avoids the duplication of code we have with set_char_table_foobar.

> How about something like this instead?
>   cset (XCHAR_TABLE (char_table), cset_parent, parent);
> where cset_parent is an offset into the structure, or
> is a member of a simple enumeration, whichever seems
> cleaner.  This would be functional and relatively
> straightforward.

Yuck!
- code duplication.
- offset into a struct instead of a plain field access?
- worse, an enum instead of a plain field access?
A non-starter.


        Stefan

bug#12215: CSET is unnecessarily confusing

[Paul Eggert]

On 08/22/2012 06:27 AM, Stefan Monnier wrote:
>> That does avoid the ambiguity but it's pretty weird.
> Less weird than CSET (XCHAR_TABLE (char_table), parent, parent),
> and avoids the duplication of code we have with set_char_table_foobar.

True in both cases.  I suppose the notation could grow on one.

A few other thoughts.  First, why would we need multiple setter
macros (CSET, BSET, etc.)?  Why can't we have just one macro?
That is, why does CSET (P, .FIELD, VAL) care what P's type is?
Surely one generic macro will do.

Second, why does the setter need the pointer to the start of
the object, as well as a pointer to the field that's changing?
Doesn't the latter suffice in a copying collector?  That is,
why can't we just turn this into something like:

   fset (&XCHAR_TABLE (char_table)->parent, parent);

?  That's shorter and simpler and avoids the need for a macro.

Third, I agree with Stefan that it'd be reasonable to put all
this setter stuff into a branch, and I'll volunteer to do
that (i.e., change back to plain assignments in the trunk,
and create a new branch called "gc" or whatever).  But I recall
that Dmitry had serious qualms about that so I would like to
hear his opinion.

bug#12215: CSET is unnecessarily confusing

[Dmitry Antipov]

On 08/22/2012 08:35 PM, Paul Eggert wrote:

> Third, I agree with Stefan that it'd be reasonable to put all
> this setter stuff into a branch, and I'll volunteer to do
> that (i.e., change back to plain assignments in the trunk,
> and create a new branch called "gc" or whatever).  But I recall
> that Dmitry had serious qualms about that so I would like to
> hear his opinion.

I'm almost convinced with going to a branch (mostly because
I have 80K patch with some GC bits, and maintaining it against
trunk becomes harder and harder).

Dmitry

bug#12215: CSET is unnecessarily confusing

[Paul Eggert]

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

On 08/22/2012 09:50 AM, Dmitry Antipov wrote:
> I'm almost convinced with going to a branch (mostly because
> I have 80K patch with some GC bits, and maintaining it against
> trunk becomes harder and harder).

OK, thanks, to try to help finish convincing you (;-)
I've prepared a patch along the lines suggested.
For example, it changes this:

  bset_directory (b, current_buffer ? BVAR (current_buffer, directory) : Qnil);

back to this:

  b->directory = current_buffer ? current_buffer->directory : Qnil;

It's a bit long (it removes over a thousand lines of code from Emacs,
net) so I've attached the compressed version.  Comments welcome.


[-- Attachment #2: remove-setters.txt.gz --]
[-- Type: application/x-gzip, Size: 112609 bytes --]

bug#12215: CSET is unnecessarily confusing

[Stefan Monnier]

> True in both cases.  I suppose the notation could grow on one.

Yes, I didn't like it much when I first thought about it, but I'm
beginning to think it's the least bad option (the next one is to use
another preprocessor than cpp ;-).

> Second, why does the setter need the pointer to the start of
> the object, as well as a pointer to the field that's changing?

Depends on how the write barrier works; more specifically, depends on
where the write-barrier writes the "this was modified" info.  One choice
is to have a flag in the object (like gcmarkbit) that says "this object
was modified since last scan"; and for that you need a pointer to the
start of the object rather than only to the field.

Of course, there are also many other choices which don't require such
a pointer (e.g. you can add the field's address to a list of "modified
fields"; or you can have a flag covering all objects in a "page", ...).


        Stefan

bug#12215: CSET is unnecessarily confusing

[Paul Eggert]

On 08/23/2012 05:26 AM, Stefan Monnier wrote:
> One choice is to have a flag in the object (like gcmarkbit)
> that says "this object was modified since last scan";
> and for that you need a pointer to the
> start of the object rather than only to the field.

If that flag has a standard name, one doesn't
need separate macros BSET/CSET/etc; one can have just
one macro that works for all types.

> there are also many other choices which don't require such
> a pointer (e.g. you can add the field's address to a list of "modified
> fields"; or you can have a flag covering all objects in a "page", ...).

Or one can rely on the hardware's memory-mapping facilities;
by making the page readonly until some code writes to it.
This would mean we don't need a setter macro.

It'd be nicer from the Emacs maintenance
point of view, most code could chug along
in the traditional way.  That is, we apply the
<http://bugs.gnu.org/12215#34> patch to the trunk to get
rid of the setters, and create a gc branch that uses mmap
rather than reintroducing lots of changes to add calls to a
setter macro or macros.

bug#12215: CSET is unnecessarily confusing

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Yes, I didn't like it much when I first thought about it, but I'm
> beginning to think it's the least bad option (the next one is to use
> another preprocessor than cpp ;-).

Maybe we could use macros that are a no-op under CPP, and serve only as
guides for a non-CPP code transformation tool.  That is to say, instead
of the horrible

  bset_directory (b, current_buffer ? BVAR (current_buffer, directory) : Qnil);

we could have the slightly less horrible

  BVAR (b->directory) = current_buffer ? BVAR (current_buffer->directory) : Qnil;

with BVAR a no-op under CPP:

  #define BVAR(x) x

The GC branch wouldn't be able to use CPP to convert this to the desired
form, but I think there would be enough information for a non-CPP tool
or a script to automatically transform all snippets of the form

  BVAR (x->y) = z

to

  set_buffer_y (x, z)

bug#12215: CSET is unnecessarily confusing

[Paul Eggert]

On 08/23/2012 08:46 PM, Chong Yidong wrote:
> Maybe we could use macros that are a no-op under CPP, and serve only as
> guides for a non-CPP code transformation tool.

How about if we use a naming convention that the code transformation
tool could pick out?  That is, instead of

  bset_directory (b, current_buffer ? BVAR (current_buffer, directory) : Qnil);

we have something like this:

  b->Directory = current_buffer ? current_buffer->Directory : Qnil;

Here the convention is that the Lisp field names start with
a capital letter.  Almost any naming convention will do.

This is straight C, so the preprocessor would not need to be
used until we have a garbage collector that needs it.

bug#12215: CSET is unnecessarily confusing

[Dmitry Antipov]

On 08/24/2012 07:57 AM, Paul Eggert wrote:
 > On 08/23/2012 08:46 PM, Chong Yidong wrote:
>> Maybe we could use macros that are a no-op under CPP, and serve only as
>> guides for a non-CPP code transformation tool.
>
> How about if we use a naming convention that the code transformation
> tool could pick out?

Please, this becomes madness. "When designing a new kind of system, a team
will design a throw-away system (whether it intends to or not)" (Brooks).
Whatever we decide now, it's likely to be changed in the future. For further
GC development, I'm quite comfortable with current Xset_xxx inline functions;
so I suggest to create GC branch starting from, say, revision 109761 of
the trunk and apply Paul's patch to remove Xset_xxx stuff from the trunk.

Dmitry

bug#12215: CSET is unnecessarily confusing

[Stefan Monnier]

Paul Eggert said:
> If that flag has a standard name, one doesn't need separate macros
> BSET/CSET/etc; one can have just one macro that works for all types.

Indeed.  Tho, since we haven't managed yet to do that for gcmarkbit, I'm
not holding my breath.

Chong Yidong said:
>   BVAR (b->directory) = current_buffer ? BVAR
>    (current_buffer->directory) : Qnil;

The BVAR accessor macro is not for the GC but for the concurrency code.
And yes, I think that BVAR(foo->bar) can be sufficient for the
concurrency code (it can be macro-expanded to buffer_var(foo->bar,
current_thread)), assuming we change all buffer slots to be of a new
type, which is a table from thread_ids to Lisp_Object.

And even if it can't be done with a macro (because we want to use
another implementation technique that needs to look up some other data
in the buffer or to know the offset of the field), having those BVAR
will make it easy to replace those accessors with something else.

For the setters, I think we'll be better off with either
BSET (b->directory, val), or BSET (b, ->directory, val), which restricts
the form that can be used.


        Stefan

bug#12215: CSET is unnecessarily confusing

[Paul Eggert]

[Tom, I'm CC'ing you as this discussion is veering into concurrency.
For context please see <http://bugs.gnu.org/12215#34>.]

On 08/24/2012 08:10 AM, Stefan Monnier wrote:
> The BVAR accessor macro is not for the GC but for the concurrency code.

How exactly does that work?  I just now looked at the concurrency branch
and its BVAR is the same as in the trunk.

Is BVAR a speculative change, that was put in because eventually we
thought we'd need it for concurrency?  If so, perhaps we should revert
it until the need is demonstrated.  After all, it's been many months
since BVAR went in, and the concurrency branch still isn't really
using it.

> And yes, I think that BVAR(foo->bar) can be sufficient for the
> concurrency code (it can be macro-expanded to buffer_var(foo->bar,
> current_thread)), assuming we change all buffer slots to be of a new
> type, which is a table from thread_ids to Lisp_Object.

In that case we shouldn't need BVAR.  Instead, we can do something
like this:

  #define backed_up backed_up_table[current_thread->id]
  #define save_length save_length_table[current_thread->id]
  ...

and then instead of this:

   bset_save_length (b, Fadd1 (BVAR (b, save_length)));

code can just go back to what it used to do:

   b->save_length = Fadd1 (b->save_length);

which is considerably more readable.

If you like, we could use a different naming convention for
these special slots, to give the reader a clue that the slots
are actually thread-local.  But the point is that we shouldn't
need BVAR or BSET.

> For the setters, I think we'll be better off with either
> BSET (b->directory, val), or BSET (b, ->directory, val), which restricts
> the form that can be used.

Wouldn't the above approach work for setters too?

The concurrency branch's BSET macro is also identical to that
of the trunk, so it's hard to see what the plan is here ...

bug#12215: CSET is unnecessarily confusing

[Tom Tromey]

>>>>> "Paul" == Paul Eggert <eggert@cs.ucla.edu> writes:

Paul> How exactly does that work?  I just now looked at the concurrency branch
Paul> and its BVAR is the same as in the trunk.

Paul> Is BVAR a speculative change, that was put in because eventually we
Paul> thought we'd need it for concurrency?  If so, perhaps we should revert
Paul> it until the need is demonstrated.  After all, it's been many months
Paul> since BVAR went in, and the concurrency branch still isn't really
Paul> using it.

Yeah.

Originally I had planned a somewhat complicated scheme for handling
per-thread let bindings.  This scheme required some changes like BVAR.
I lobbied to get these changes into the trunk because carrying big
changes like this on a branch makes merging very hard -- this is partly
what sunk the first concurrency branch.

My planned bindings approach was tricky, though, and due to unrelated
changes in my life, I no longer have the time or energy to think through
all the details to make them work with other changes that have happened
in Emacs since then.  So on the new concurrency branch I opted for a
much simpler approach (which may still be wrong :-).

BVAR isn't needed right now.  It may be if someone else resurrects the
other approach.  It's unlikely I ever will.

Tom

bug#12215: CSET is unnecessarily confusing

[Paul Eggert]

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

On 08/24/2012 10:27 AM, Tom Tromey wrote:
> BVAR isn't needed right now.  It may be if someone else resurrects the
> other approach.  It's unlikely I ever will.

Thanks for the quick response.  It seems that everybody here
(Tom, Dmitry, Chong, Stefan) is on board with reverting
the setter functions and accessor macros in the trunk.  I've merged
recent trunk changes (up to trunk bzr 109763) into the proposed
patch to do that, and attach a copy.  (The full patch is about 500 kB
so the attachment is compressed.)  I assume there's no objection to
installing this but will hold off for a couple of days to allow for
further comment.


[-- Attachment #2: no-setters.txt.xz --]
[-- Type: application/x-xz, Size: 89764 bytes --]

bug#12215: CSET is unnecessarily confusing

[Stefan Monnier]

> BVAR isn't needed right now.  It may be if someone else resurrects the
> other approach.  It's unlikely I ever will.

While it's not needed for the current concurrency branch, and I think
this implementation is fine for a first effort at adding concurrency,
I do believe that it will have to change in the longer term.

> Thanks for the quick response.  It seems that everybody here
> (Tom, Dmitry, Chong, Stefan) is on board with reverting
> the setter functions and accessor macros in the trunk.

No.  I'm actually quite happy keeping xVAR accessor macros (for
let-bindable vars that will/may require special handling in a future
concurrency implementation) and xSET setter macros (for let-bindable
vars and/or for write barriers), to make it easier to experiment
on branches.
But I'd like those macros to have a shape that we can live with.


        Stefan

bug#12215: CSET is unnecessarily confusing

[Paul Eggert]

On 08/24/2012 02:12 PM, Stefan Monnier wrote:
> I'm actually quite happy keeping xVAR accessor macros ... and xSET
> setter macros ..., to make it easier to experiment on branches.

Ah, sorry, I misunderstood.  But still, currently the people
who are actually doing those experiments (Dmitry, Tom) don't
need these macros and don't particularly want them.  And
Chong is calling BVAR "horrible".  And I too would rather
avoid these macros absent a proven need for them.  If
there's anything that Dmitry's and Tom's experiences have
shown, it's that speculative changes often don't pan out.

We currently have a patch that will get rid of the setters
and of the xVAR and xSET macros, reverting to the pre-23.3
coding style.  With some more work, I can change this patch
to keep the BVAR and KVAR macros, reverting it to the 24.2
coding style.  With still more work I could introduce xSET
macros (assuming we come up on a style for them), resulting
in a new style.

I'd like to avoid this extra work if possible, so how about
this idea for moving forward?  I'll install the
abovementioned patch.  If anyone actually needs the xVAR
and/or xSET macros, I'll volunteer to do the tedious work to
put them into the trunk.  (This offer is good for one year
or 10,000 edits, whichever comes first. :-)

That way, if we don't need those macros I'll save some work,
and if we do need them I won't cost myself any more work
than I'd do under your proposal.

bug#12215: CSET is unnecessarily confusing

[Stefan Monnier]

> Ah, sorry, I misunderstood.  But still, currently the people
> who are actually doing those experiments (Dmitry, Tom) don't
> need these macros and don't particularly want them.

AFAIK that's true of BVAR but not of setter macros (which Dmitry does
need).

> And Chong is calling BVAR "horrible".

Yes, tho the issue is the use of field names in syntactic contexts where
they can be confused for variable names.


        Stefan

bug#12215: CSET is unnecessarily confusing

[Paul Eggert]

>> currently the people who are actually doing those experiments
>> (Dmitry, Tom) don't need these macros and don't particularly want them.
> 
> AFAIK that's true of BVAR but not of setter macros (which Dmitry does need).

Dmitry suggested in <http://bugs.gnu.org/12215#49> to remove the setters
from the trunk, and that he try them out in the GC branch.
Although Dmitry wrote that he's quite comfortable with setter functions
that are currently in the trunk, the setters could be also macros using
the syntax that you're thinking of.  Either way, Dmitry could gain
experience with setters in the GC branch, and we wouldn't have to decide
now how setters should work in the trunk.
 
>> And Chong is calling BVAR "horrible".
> 
> Yes, tho the issue is the use of field names in syntactic contexts where
> they can be confused for variable names.

That's the main issue, I guess.  But BVAR also makes code harder to read.