Re: [Emacs-diffs] master b7fa6b1 1/4: Simplify use of FOR_EACH_TAIL

Re: [Emacs-diffs] master b7fa6b1 1/4: Simplify use of FOR_EACH_TAIL

[Stefan Monnier]

> -  FOR_EACH_TAIL (tail, list, tortoise, skip)
> +  FOR_EACH_TAIL (list)

I like the remove of `skip` and `tortoise` since they're really internal
and not expected to be used by users of the macro, but I'm surprised you
also decided to remove `tail`, since that one *is* used by the users of
the macro and forcing them to use `li.tail` instead of the variable of
their choice is rather odd.


        Stefan

Re: [Emacs-diffs] master b7fa6b1 1/4: Simplify use of FOR_EACH_TAIL

[Paul Eggert]

Stefan Monnier wrote:
> I'm surprised you
> also decided to remove `tail`, since that one *is* used by the users of
> the macro and forcing them to use `li.tail` instead of the variable of
> their choice is rather odd.

Logically, the variable is part of the iterator, not of the surrounding code. I 
did at first write a macro along the lines that you suggest, and also tried 
another variant where the user could specify a name of their choice for the 
iterator, but in both cases this extra flexibility made the code a bit more 
verbose and was not that useful in practice. It would be easy to resurrect this 
flexibility if needed (it doesn't affect efficiency, of course).

Here's an example. At first, I wrote a revised version like this:

   Lisp_Object tail;
   FOR_EACH_TAIL (tail, list)
     if (EQ (XCAR (tail), elt))
       return true;

but this was annoying both because the user must declare 'tail', and because 
typically 'tail' should not survive the loop but the declaration forces it to 
survive. I wanted something more like this:

   FOR_EACH_TAIL (tail, list)
     if (EQ (XCAR (tail), elt))
       return true;

where the macro declares 'tail' to be local to the loop. Unfortunately there 
doesn't seem to be any way to that in portable C. So I settled for this:

   FOR_EACH_TAIL (list)
     if (EQ (XCAR (li.tail), elt))
       return true;

which is even shorter than what I wanted, albeit with an 'li.tail' that looks 
odd at least at first. Although we could easily change things to let the caller 
specify a name, like this:

   FOR_EACH_TAIL (li, list)
     if (EQ (XCAR (li.tail), elt))
       return true;

it's rare to need different names for iterators, so it's unclear that this extra 
complexity would be helpful.

Re: [Emacs-diffs] master b7fa6b1 1/4: Simplify use of FOR_EACH_TAIL

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 5 Feb 2017 21:23:43 -0800
> 
>    Lisp_Object tail;
>    FOR_EACH_TAIL (tail, list)
>      if (EQ (XCAR (tail), elt))
>        return true;
> 
> but this was annoying both because the user must declare 'tail', and because 
> typically 'tail' should not survive the loop but the declaration forces it to 
> survive.

Why is it so important to make sure it doesn't survive the loop?  If
for optimizations, then I'd expect modern compilers to be smart enough
to produce the same code whether it does or doesn't survive.

We have similar macros elsewhere, e.g. FOR_EACH_FRAME,
FOR_EACH_BUFFER, etc.  It's IMO a pity to have this one different.

Re: [Emacs-diffs] master b7fa6b1 1/4: Simplify use of FOR_EACH_TAIL

[Paul Eggert]

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

On 02/06/2017 07:37 AM, Eli Zaretskii wrote:
> Why is it so important to make sure it doesn't survive the loop?  If
> for optimizations, then I'd expect modern compilers to be smart enough
> to produce the same code whether it does or doesn't survive.

It's not so much compilers as human readers of the code. Admittedly it 
is a minor issue.

> We have similar macros elsewhere, e.g. FOR_EACH_FRAME,
> FOR_EACH_BUFFER, etc.  It's IMO a pity to have this one different.

Good point, I had forgotten about that. I installed the attached patch, 
which changes FOR_EACH_TAIL to act more like these other macros. That 
is. FOR_EACH_TAIL (tail) now updates 'tail' each time through the loop, 
and the caller needs to declare the 'tail' variable outside the loop; 
this is as it was before.


[-- Attachment #2: 0001-Make-FOR_EACH_TAIL-more-like-other-FOR_EACH-macros.txt --]
[-- Type: text/plain, Size: 15959 bytes --]

From 2b7edea1c2559b79f52d0094b5ba2cd0d6700bd2 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 6 Feb 2017 17:15:14 -0800
Subject: [PATCH] Make FOR_EACH_TAIL more like other FOR_EACH macros
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

See comments by Stefan Monnier in:
http://lists.gnu.org/archive/html/emacs-devel/2017-02/msg00181.html
and by Eli Zaretskii in:
http://lists.gnu.org/archive/html/emacs-devel/2017-02/msg00207.html
* src/fns.c (internal_equal): Do not bypass check for depth
overflow when tail-recursing via a dotted list tail or an overlay
plist, to avoid a rare infloop.
* src/lisp.h (FOR_EACH_TAIL, FOR_EACH_TAIL_SAFE): Take TAIL as an
arg, and update it at each iteration, rather than have callers
access it.tail.  All callers changed.
(FOR_EACH_TAIL): Do not check for dotted lists, as this is now
the caller’s responsibility.  All callers changed.
(FOR_EACH_TAIL_CONS): Remove.  All callers changed.
(struct for_each_tail_internal.tail): Remove; no longer needed.
(FOR_EACH_TAIL_INTERNAL): Remove dotted arg, and set the tail
arg each time through the loop.  All callers changed.
---
 src/fns.c   | 182 +++++++++++++++++++++++++++++++++---------------------------
 src/lisp.h  |  48 +++++++---------
 src/xdisp.c |   4 +-
 3 files changed, 123 insertions(+), 111 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index b5508fb..ffe3218 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -111,6 +111,7 @@ To get the number of bytes, use `string-bytes'.  */)
       intptr_t i = 0;
       FOR_EACH_TAIL (sequence)
 	i++;
+      CHECK_LIST_END (sequence, sequence);
       if (MOST_POSITIVE_FIXNUM < i)
 	error ("List too long");
       val = make_number (i);
@@ -1343,9 +1344,11 @@ DEFUN ("member", Fmember, Smember, 2, 2, 0,
 The value is actually the tail of LIST whose car is ELT.  */)
   (Lisp_Object elt, Lisp_Object list)
 {
-  FOR_EACH_TAIL (list)
-    if (! NILP (Fequal (elt, XCAR (li.tail))))
-      return li.tail;
+  Lisp_Object tail = list;
+  FOR_EACH_TAIL (tail)
+    if (! NILP (Fequal (elt, XCAR (tail))))
+      return tail;
+  CHECK_LIST_END (tail, list);
   return Qnil;
 }
 
@@ -1354,9 +1357,11 @@ DEFUN ("memq", Fmemq, Smemq, 2, 2, 0,
 The value is actually the tail of LIST whose car is ELT.  */)
   (Lisp_Object elt, Lisp_Object list)
 {
-  FOR_EACH_TAIL (list)
-    if (EQ (XCAR (li.tail), elt))
-      return li.tail;
+  Lisp_Object tail = list;
+  FOR_EACH_TAIL (tail)
+    if (EQ (XCAR (tail), elt))
+      return tail;
+  CHECK_LIST_END (tail, list);
   return Qnil;
 }
 
@@ -1368,12 +1373,14 @@ The value is actually the tail of LIST whose car is ELT.  */)
   if (!FLOATP (elt))
     return Fmemq (elt, list);
 
-  FOR_EACH_TAIL (list)
+  Lisp_Object tail = list;
+  FOR_EACH_TAIL (tail)
     {
-      Lisp_Object tem = XCAR (li.tail);
+      Lisp_Object tem = XCAR (tail);
       if (FLOATP (tem) && internal_equal (elt, tem, 0, 0, Qnil))
-	return li.tail;
+	return tail;
     }
+  CHECK_LIST_END (tail, list);
   return Qnil;
 }
 
@@ -1383,9 +1390,11 @@ The value is actually the first element of LIST whose car is KEY.
 Elements of LIST that are not conses are ignored.  */)
   (Lisp_Object key, Lisp_Object list)
 {
-  FOR_EACH_TAIL (list)
-    if (CONSP (XCAR (li.tail)) && EQ (XCAR (XCAR (li.tail)), key))
-      return XCAR (li.tail);
+  Lisp_Object tail = list;
+  FOR_EACH_TAIL (tail)
+    if (CONSP (XCAR (tail)) && EQ (XCAR (XCAR (tail)), key))
+      return XCAR (tail);
+  CHECK_LIST_END (tail, list);
   return Qnil;
 }
 
@@ -1406,13 +1415,15 @@ DEFUN ("assoc", Fassoc, Sassoc, 2, 2, 0,
 The value is actually the first element of LIST whose car equals KEY.  */)
   (Lisp_Object key, Lisp_Object list)
 {
-  FOR_EACH_TAIL (list)
+  Lisp_Object tail = list;
+  FOR_EACH_TAIL (tail)
     {
-      Lisp_Object car = XCAR (li.tail);
+      Lisp_Object car = XCAR (tail);
       if (CONSP (car)
 	  && (EQ (XCAR (car), key) || !NILP (Fequal (XCAR (car), key))))
 	return car;
     }
+  CHECK_LIST_END (tail, list);
   return Qnil;
 }
 
@@ -1437,9 +1448,11 @@ DEFUN ("rassq", Frassq, Srassq, 2, 2, 0,
 The value is actually the first element of LIST whose cdr is KEY.  */)
   (Lisp_Object key, Lisp_Object list)
 {
-  FOR_EACH_TAIL (list)
-    if (CONSP (XCAR (li.tail)) && EQ (XCDR (XCAR (li.tail)), key))
-      return XCAR (li.tail);
+  Lisp_Object tail = list;
+  FOR_EACH_TAIL (tail)
+    if (CONSP (XCAR (tail)) && EQ (XCDR (XCAR (tail)), key))
+      return XCAR (tail);
+  CHECK_LIST_END (tail, list);
   return Qnil;
 }
 
@@ -1448,13 +1461,15 @@ DEFUN ("rassoc", Frassoc, Srassoc, 2, 2, 0,
 The value is actually the first element of LIST whose cdr equals KEY.  */)
   (Lisp_Object key, Lisp_Object list)
 {
-  FOR_EACH_TAIL (list)
+  Lisp_Object tail = list;
+  FOR_EACH_TAIL (tail)
     {
-      Lisp_Object car = XCAR (li.tail);
+      Lisp_Object car = XCAR (tail);
       if (CONSP (car)
 	  && (EQ (XCDR (car), key) || !NILP (Fequal (XCDR (car), key))))
 	return car;
     }
+  CHECK_LIST_END (tail, list);
   return Qnil;
 }
 \f
@@ -1470,21 +1485,22 @@ the value of a list `foo'.  See also `remq', which does not modify the
 argument.  */)
   (Lisp_Object elt, Lisp_Object list)
 {
-  Lisp_Object prev = Qnil;
+  Lisp_Object prev = Qnil, tail = list;
 
-  FOR_EACH_TAIL (list)
+  FOR_EACH_TAIL (tail)
     {
-      Lisp_Object tem = XCAR (li.tail);
+      Lisp_Object tem = XCAR (tail);
       if (EQ (elt, tem))
 	{
 	  if (NILP (prev))
-	    list = XCDR (li.tail);
+	    list = XCDR (tail);
 	  else
-	    Fsetcdr (prev, XCDR (li.tail));
+	    Fsetcdr (prev, XCDR (tail));
 	}
       else
-	prev = li.tail;
+	prev = tail;
     }
+  CHECK_LIST_END (tail, list);
   return list;
 }
 
@@ -1592,20 +1608,21 @@ changing the value of a sequence `foo'.  */)
     }
   else
     {
-      Lisp_Object prev = Qnil;
+      Lisp_Object prev = Qnil, tail = seq;
 
-      FOR_EACH_TAIL (seq)
+      FOR_EACH_TAIL (tail)
 	{
-	  if (!NILP (Fequal (elt, (XCAR (li.tail)))))
+	  if (!NILP (Fequal (elt, XCAR (tail))))
 	    {
 	      if (NILP (prev))
-		seq = XCDR (li.tail);
+		seq = XCDR (tail);
 	      else
-		Fsetcdr (prev, XCDR (li.tail));
+		Fsetcdr (prev, XCDR (tail));
 	    }
 	  else
-	    prev = li.tail;
+	    prev = tail;
 	}
+      CHECK_LIST_END (tail, seq);
     }
 
   return seq;
@@ -1678,7 +1695,8 @@ See also the function `nreverse', which is used more often.  */)
     {
       new = Qnil;
       FOR_EACH_TAIL (seq)
-	new = Fcons (XCAR (li.tail), new);
+	new = Fcons (XCAR (seq), new);
+      CHECK_LIST_END (seq, seq);
     }
   else if (VECTORP (seq))
     {
@@ -1930,14 +1948,15 @@ corresponding to the given PROP, or nil if PROP is not one of the
 properties on the list.  This function never signals an error.  */)
   (Lisp_Object plist, Lisp_Object prop)
 {
-  FOR_EACH_TAIL_SAFE (plist)
+  Lisp_Object tail = plist;
+  FOR_EACH_TAIL_SAFE (tail)
     {
-      if (! CONSP (XCDR (li.tail)))
+      if (! CONSP (XCDR (tail)))
 	break;
-      if (EQ (prop, XCAR (li.tail)))
-	return XCAR (XCDR (li.tail));
-      li.tail = XCDR (li.tail);
-      if (EQ (li.tail, li.tortoise))
+      if (EQ (prop, XCAR (tail)))
+	return XCAR (XCDR (tail));
+      tail = XCDR (tail);
+      if (EQ (tail, li.tortoise))
 	break;
     }
 
@@ -1963,23 +1982,24 @@ use `(setq x (plist-put x prop val))' to be sure to use the new value.
 The PLIST is modified by side effects.  */)
   (Lisp_Object plist, Lisp_Object prop, Lisp_Object val)
 {
-  Lisp_Object prev = Qnil;
-  FOR_EACH_TAIL_CONS (plist)
+  Lisp_Object prev = Qnil, tail = plist;
+  FOR_EACH_TAIL (tail)
     {
-      if (! CONSP (XCDR (li.tail)))
+      if (! CONSP (XCDR (tail)))
 	break;
 
-      if (EQ (prop, XCAR (li.tail)))
+      if (EQ (prop, XCAR (tail)))
 	{
-	  Fsetcar (XCDR (li.tail), val);
+	  Fsetcar (XCDR (tail), val);
 	  return plist;
 	}
 
-      prev = li.tail;
-      li.tail = XCDR (li.tail);
-      if (EQ (li.tail, li.tortoise))
+      prev = tail;
+      tail = XCDR (tail);
+      if (EQ (tail, li.tortoise))
 	circular_list (plist);
     }
+  CHECK_LIST_END (tail, plist);
   Lisp_Object newcell
     = Fcons (prop, Fcons (val, NILP (prev) ? plist : XCDR (XCDR (prev))));
   if (NILP (prev))
@@ -2007,16 +2027,20 @@ corresponding to the given PROP, or nil if PROP is not
 one of the properties on the list.  */)
   (Lisp_Object plist, Lisp_Object prop)
 {
-  FOR_EACH_TAIL_CONS (plist)
+  Lisp_Object tail = plist;
+  FOR_EACH_TAIL (tail)
     {
-      if (! CONSP (XCDR (li.tail)))
+      if (! CONSP (XCDR (tail)))
 	break;
-      if (! NILP (Fequal (prop, XCAR (li.tail))))
-	return XCAR (XCDR (li.tail));
-      li.tail = XCDR (li.tail);
-      if (EQ (li.tail, li.tortoise))
+      if (! NILP (Fequal (prop, XCAR (tail))))
+	return XCAR (XCDR (tail));
+      tail = XCDR (tail);
+      if (EQ (tail, li.tortoise))
 	circular_list (plist);
     }
+
+  CHECK_LIST_END (tail, plist);
+
   return Qnil;
 }
 
@@ -2030,23 +2054,24 @@ use `(setq x (lax-plist-put x prop val))' to be sure to use the new value.
 The PLIST is modified by side effects.  */)
   (Lisp_Object plist, Lisp_Object prop, Lisp_Object val)
 {
-  Lisp_Object prev = Qnil;
-  FOR_EACH_TAIL_CONS (plist)
+  Lisp_Object prev = Qnil, tail = plist;
+  FOR_EACH_TAIL (tail)
     {
-      if (! CONSP (XCDR (li.tail)))
+      if (! CONSP (XCDR (tail)))
 	break;
 
-      if (! NILP (Fequal (prop, XCAR (li.tail))))
+      if (! NILP (Fequal (prop, XCAR (tail))))
 	{
-	  Fsetcar (XCDR (li.tail), val);
+	  Fsetcar (XCDR (tail), val);
 	  return plist;
 	}
 
-      prev = li.tail;
-      li.tail = XCDR (li.tail);
-      if (EQ (li.tail, li.tortoise))
+      prev = tail;
+      tail = XCDR (tail);
+      if (EQ (tail, li.tortoise))
 	circular_list (plist);
     }
+  CHECK_LIST_END (tail, plist);
   Lisp_Object newcell = list2 (prop, val);
   if (NILP (prev))
     return newcell;
@@ -2095,6 +2120,7 @@ static bool
 internal_equal (Lisp_Object o1, Lisp_Object o2, int depth, bool props,
 		Lisp_Object ht)
 {
+ tail_recurse:
   if (depth > 10)
     {
       if (depth > 200)
@@ -2123,7 +2149,6 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, int depth, bool props,
 	}
     }
 
- tail_recurse:
   if (EQ (o1, o2))
     return 1;
   if (XTYPE (o1) != XTYPE (o2))
@@ -2144,20 +2169,16 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, int depth, bool props,
 
     case Lisp_Cons:
       {
-	Lisp_Object tail1 = o1;
-	FOR_EACH_TAIL_CONS (o1)
+	FOR_EACH_TAIL (o1)
 	  {
 	    if (! CONSP (o2))
 	      return false;
-	    if (! internal_equal (XCAR (li.tail), XCAR (o2), depth + 1,
-				  props, ht))
+	    if (! internal_equal (XCAR (o1), XCAR (o2), depth + 1, props, ht))
 	      return false;
-	    tail1 = XCDR (li.tail);
 	    o2 = XCDR (o2);
-	    if (EQ (tail1, o2))
+	    if (EQ (XCDR (o1), o2))
 	      return true;
 	  }
-	o1 = tail1;
 	depth++;
 	goto tail_recurse;
       }
@@ -2340,8 +2361,8 @@ usage: (nconc &rest LISTS)  */)
       CHECK_CONS (tem);
 
       Lisp_Object tail;
-      FOR_EACH_TAIL_CONS (tem)
-	tail = li.tail;
+      FOR_EACH_TAIL (tem)
+	tail = tem;
 
       tem = args[argnum + 1];
       Fsetcdr (tail, tem);
@@ -2763,19 +2784,18 @@ property and a property with the value nil.
 The value is actually the tail of PLIST whose car is PROP.  */)
   (Lisp_Object plist, Lisp_Object prop)
 {
-  FOR_EACH_TAIL (plist)
+  Lisp_Object tail = plist;
+  FOR_EACH_TAIL (tail)
     {
-      if (EQ (XCAR (li.tail), prop))
-	return li.tail;
-      if (!CONSP (XCDR (li.tail)))
-	{
-	  CHECK_LIST_END (XCDR (li.tail), plist);
-	  return Qnil;
-	}
-      li.tail = XCDR (li.tail);
-      if (EQ (li.tail, li.tortoise))
-	circular_list (plist);
+      if (EQ (XCAR (tail), prop))
+	return tail;
+      tail = XCDR (tail);
+      if (! CONSP (tail))
+	break;
+      if (EQ (tail, li.tortoise))
+	circular_list (tail);
     }
+  CHECK_LIST_END (tail, plist);
   return Qnil;
 }
 
diff --git a/src/lisp.h b/src/lisp.h
index b753971..f1e2685 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4580,41 +4580,33 @@ enum
 	 Lisp_String))							\
      : make_unibyte_string (str, len))
 
-/* Loop over tails of LIST, checking for dotted lists and cycles,
-   and possibly quitting after each loop iteration.
-   In the loop body, ‘li.tail’ is the current cons; the name ‘li’ is
-   short for “list iterator”.  The expression LIST may be evaluated
-   more than once, and so should not have side effects.  The loop body
+/* Loop over conses of the list TAIL, signaling if a cycle is found,
+   and possibly quitting after each loop iteration.  In the loop body,
+   set TAIL to the current cons.  If the loop exits normally,
+   set TAIL to the terminating non-cons, typically nil.  The loop body
    should not modify the list’s top level structure other than by
    perhaps deleting the current cons.  */
 
-#define FOR_EACH_TAIL(list)						\
-  FOR_EACH_TAIL_INTERNAL (list, CHECK_LIST_END (li.tail, list),		\
-			  circular_list (list), true)
+#define FOR_EACH_TAIL(tail) \
+  FOR_EACH_TAIL_INTERNAL (tail, circular_list (tail), true)
 
-/* Like FOR_EACH_TAIL (LIST), except do not check for dotted lists.  */
+/* Like FOR_EACH_TAIL (LIST), except do not signal or quit.
+   If the loop exits due to a cycle, TAIL’s value is undefined.  */
 
-#define FOR_EACH_TAIL_CONS(list)				\
-  FOR_EACH_TAIL_INTERNAL (list, (void) 0, circular_list (list), true)
-
-/* Like FOR_EACH_TAIL (LIST), except check for neither dotted lists
-   nor cycles, and do not quit.  */
-
-#define FOR_EACH_TAIL_SAFE(list) \
-  FOR_EACH_TAIL_INTERNAL (list, (void) 0, (void) (li.tail = Qnil), false)
+#define FOR_EACH_TAIL_SAFE(tail) \
+  FOR_EACH_TAIL_INTERNAL (tail, (void) ((tail) = Qnil), false)
 
 /* Iterator intended for use only within FOR_EACH_TAIL_INTERNAL.  */
 struct for_each_tail_internal
 {
-  Lisp_Object tail, tortoise;
+  Lisp_Object tortoise;
   intptr_t max, n;
   unsigned short int q;
 };
 
-/* Like FOR_EACH_TAIL (LIST), except evaluate DOTTED or CYCLE,
-   respectively, if a dotted list or cycle is found, and check for
-   quit if CHECK_QUIT.  This is an internal macro intended for use
-   only by the above macros.
+/* Like FOR_EACH_TAIL (LIST), except evaluate CYCLE if a cycle is
+   found, and check for quit if CHECK_QUIT.  This is an internal macro
+   intended for use only by the above macros.
 
    Use Brent’s teleporting tortoise-hare algorithm.  See:
    Brent RP. BIT. 1980;20(2):176-84. doi:10.1007/BF01933190
@@ -4626,15 +4618,15 @@ struct for_each_tail_internal
    other noninterruptible areas (e.g., garbage collection) that there
    is little point to calling maybe_quit here.  */
 
-#define FOR_EACH_TAIL_INTERNAL(list, dotted, cycle, check_quit)		\
-  for (struct for_each_tail_internal li = { list, list, 2, 0, 2 };	\
-       CONSP (li.tail) || (dotted, false);				\
-       (li.tail = XCDR (li.tail),					\
+#define FOR_EACH_TAIL_INTERNAL(tail, cycle, check_quit)			\
+  for (struct for_each_tail_internal li = { tail, 2, 0, 2 };		\
+       CONSP (tail);							\
+       ((tail) = XCDR (tail),						\
 	((--li.q != 0							\
 	  || ((check_quit) ? maybe_quit () : (void) 0, 0 < --li.n)	\
 	  || (li.q = li.n = li.max <<= 1, li.n >>= USHRT_WIDTH,		\
-	      li.tortoise = li.tail, false))				\
-	 && EQ (li.tail, li.tortoise))					\
+	      li.tortoise = (tail), false))				\
+	 && EQ (tail, li.tortoise))					\
 	? (cycle) : (void) 0))
 
 /* Do a `for' loop over alist values.  */
diff --git a/src/xdisp.c b/src/xdisp.c
index 5e1207f..387a370 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -23040,10 +23040,10 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 	      n += display_mode_element (it, depth,
 					 /* Pad after only the last
 					    list element.  */
-					 (! CONSP (XCDR (li.tail))
+					 (! CONSP (XCDR (elt))
 					  ? field_width - n
 					  : 0),
-					 precision - n, XCAR (li.tail),
+					 precision - n, XCAR (elt),
 					 props, risky);
 	    }
       }
-- 
2.9.3

Re: [Emacs-diffs] master b7fa6b1 1/4: Simplify use of FOR_EACH_TAIL

[Eli Zaretskii]

> Cc: monnier@IRO.UMontreal.CA, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 6 Feb 2017 17:35:51 -0800
> 
> > We have similar macros elsewhere, e.g. FOR_EACH_FRAME,
> > FOR_EACH_BUFFER, etc.  It's IMO a pity to have this one different.
> 
> Good point, I had forgotten about that. I installed the attached patch, 
> which changes FOR_EACH_TAIL to act more like these other macros. That 
> is. FOR_EACH_TAIL (tail) now updates 'tail' each time through the loop, 
> and the caller needs to declare the 'tail' variable outside the loop; 
> this is as it was before.

Thanks.

Re: [Emacs-diffs] master b7fa6b1 1/4: Simplify use of FOR_EACH_TAIL

[Davis Herring]

>   FOR_EACH_TAIL (tail, list)
>     if (EQ (XCAR (tail), elt))
>       return true;
>
> where the macro declares 'tail' to be local to the loop. Unfortunately
> there doesn't seem to be any way to that in portable C.

Could you explain that limitation?  I thought you meant by "portable C" 
C89, but you of course used a C99 loop-scope declaration in the end. 
Did you just mean that you couldn't have that variable declared and also 
the tortoise integer declared because they were different types (and you 
didn't want to open a pair of braces in the macro that would have to be 
closed later)?

Mostly curious,
Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or 
too sparse, it is because mass-energy conversion has occurred during 
shipping.

Re: [Emacs-diffs] master b7fa6b1 1/4: Simplify use of FOR_EACH_TAIL

[Paul Eggert]

On 02/06/2017 09:38 AM, Davis Herring wrote:
> Did you just mean that you couldn't have that variable declared and 
> also the tortoise integer declared because they were different types 
> (and you didn't want to open a pair of braces in the macro that would 
> have to be closed later)? 

Yes, that sort of thing, although it's not that they are different types.

Portable C99 code cannot declare two or more identifiers in the same 
for-loop, due to an ambiguity in the C99 standard, which says (6.8.5 
para 3) "The declaration part of a for statement shall only declare 
identifiers for objects having storage class auto or register." This 
constraint can be interpreted as meaning (A) when a for-statement 
declares an object's name, the name must have storage class auto or 
register, or (B) all identifiers declared by a for-statement must name 
objects with storage class auto or register. Although I suspect (A) was 
intended and that's what GCC does, (B) is also a plausible 
interpretation and that's what Clang does. Interpretation (B) disallows 
anonymous structs here, as such structs declare member names.

Re: [Emacs-diffs] master b7fa6b1 1/4: Simplify use of FOR_EACH_TAIL

[Johan Bockgård]

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

> Portable C99 code cannot declare two or more identifiers in the same
> for-loop, due to an ambiguity in the C99 standard, which says (6.8.5
> para 3) "The declaration part of a for statement shall only declare
> identifiers for objects having storage class auto or register." This
> constraint can be interpreted as meaning (A) when a for-statement
> declares an object's name, the name must have storage class auto or
> register, or (B) all identifiers declared by a for-statement must name
> objects with storage class auto or register. Although I suspect (A)
> was intended and that's what GCC does, (B) is also a plausible
> interpretation and that's what Clang does. Interpretation (B)
> disallows anonymous structs here, as such structs declare member
> names.

    "The intent is clear enough; [...] identifiers which do not denote
    objects with auto or register storage classes, [...] are not allowed
    in this context."

http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_277.htm

Re: [Emacs-diffs] master b7fa6b1 1/4: Simplify use of FOR_EACH_TAIL

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > constraint can be interpreted as meaning (A) when a for-statement 
  > declares an object's name, the name must have storage class auto or 
  > register, or (B) all identifiers declared by a for-statement must name 
  > objects with storage class auto or register. Although I suspect (A) was 
  > intended and that's what GCC does, (B) is also a plausible 
  > interpretation and that's what Clang does. Interpretation (B) disallows 
  > anonymous structs here, as such structs declare member names.

Why should we restrict ourselves to cater to Clang?
That makes no sense for us.

We should write this code the right way, and recommend that
people compile with GCC.  People who want to compile with Clang
can cope in one way or another.

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

Re: [Emacs-diffs] master b7fa6b1 1/4: Simplify use of FOR_EACH_TAIL

[Paul Eggert]

On 02/07/2017 01:50 PM, Richard Stallman wrote:
> Why should we restrict ourselves to cater to Clang?

It's not just Clang, it's any compiler that interprets that part of the 
C standard strictly. I have heard that Visual Studio is like Clang here, 
and I wouldn't be surprised if other compilers were in the strict camp.

Since there is a short and clean workaround for this that does not hurt 
performance, it should be OK for Emacs to use the workaround. After all, 
Emacs has localized workarounds elsewhere for proprietary compilers like 
AIX xlc, HP-UX cc and Oracle Studio cc.