GNU bug report logs - #13419
make 'eabs' act more like a function

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 13419 in the body.
You can then email your comments to 13419 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-gnu-emacs <at> gnu.org
Subject: make 'eabs' act more like a function
Date: Fri, 11 Jan 2013 22:44:19 -0800

Tags: patch
Severity: minor

This patch doesn't fix any bugs, it's just a code cleanup.
It fixes 'eabs (X)' so that it evaluates X just once
instead of twice.  As Emacs is currently written this shouldn't
affect efficiency or behavior, but it is a nicety.

The key part of this patch is to lisp.h; most of the rest
is pulled in from gnulib automatically.  I'm guessing about
the change to nt/inc/inttypes.h, as I don't use Windows.

=== modified file 'ChangeLog'
--- ChangeLog	2013-01-12 05:21:06 +0000
+++ ChangeLog	2013-01-12 06:31:32 +0000
@@ -1,5 +1,10 @@
 2013-01-12  Paul Eggert  <eggert <at> cs.ucla.edu>
 
+	Make eabs act more like a function.
+	* configure.ac: Invoke AC_C_TYPEOF.
+	* lib/imaxabs.c, m4/imaxabs.m4, m4/_Generic.m4: New files, from gnulib.
+	* lib/gnulib.mk, m4/gnulib-comp.m4: Regenerate.
+
 	Enable conservative stack scanning for all architectures.
 	Suggested by Stefan Monnier in
 	<http://lists.gnu.org/archive/html/emacs-devel/2013-01/msg00183.html>.

=== modified file 'admin/ChangeLog'
--- admin/ChangeLog	2013-01-10 03:43:02 +0000
+++ admin/ChangeLog	2013-01-12 01:03:44 +0000
@@ -1,3 +1,8 @@
+2013-01-12  Paul Eggert  <eggert <at> cs.ucla.edu>
+
+	Make eabs act more like a function.
+	* merge-gnulib (GNULIB_MODULES): Add _Generic, imaxabs.
+
 2013-01-03  Glenn Morris  <rgm <at> gnu.org>
 
 	* check-doc-strings: Update for CVS->bzr, moved lispref/ directory.

=== modified file 'admin/merge-gnulib'
--- admin/merge-gnulib	2013-01-02 16:13:04 +0000
+++ admin/merge-gnulib	2013-01-12 01:03:44 +0000
@@ -26,11 +26,11 @@
 GNULIB_URL=git://git.savannah.gnu.org/gnulib.git
 
 GNULIB_MODULES='
-  alloca-opt c-ctype c-strcase
+  _Generic alloca-opt c-ctype c-strcase
   careadlinkat close-stream crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512
   dtoastr dtotimespec dup2 environ execinfo faccessat
   fcntl-h filemode getloadavg getopt-gnu gettime gettimeofday
-  ignore-value intprops largefile lstat
+  ignore-value imaxabs intprops largefile lstat
   manywarnings mktime pselect pthread_sigmask putenv readlink
   sig2str socklen stat-time stdalign stdarg stdbool stdio
   strftime strtoimax strtoumax symlink sys_stat

=== modified file 'configure.ac'
--- configure.ac	2013-01-12 05:21:06 +0000
+++ configure.ac	2013-01-12 06:31:32 +0000
@@ -3350,6 +3350,8 @@
 
 AC_TYPE_MBSTATE_T
 
+AC_C_TYPEOF
+
 AC_CACHE_CHECK([for C restricted array declarations], emacs_cv_c_restrict_arr,
   [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[void fred (int x[__restrict]);]], [[]])],
                   emacs_cv_c_restrict_arr=yes, emacs_cv_c_restrict_arr=no)])

=== modified file 'lib/gnulib.mk'
--- lib/gnulib.mk	2013-01-04 02:17:49 +0000
+++ lib/gnulib.mk	2013-01-12 01:03:44 +0000
@@ -21,7 +21,7 @@
 # the same distribution terms as the rest of that program.
 #
 # Generated by gnulib-tool.
-# Reproduce by: gnulib-tool --import --dir=. --lib=libgnu --source-base=lib --m4-base=m4 --doc-base=doc --tests-base=tests --aux-dir=build-aux --avoid=at-internal --avoid=errno --avoid=fchdir --avoid=fcntl --avoid=fstat --avoid=malloc-posix --avoid=msvc-inval --avoid=msvc-nothrow --avoid=openat-die --avoid=openat-h --avoid=raise --avoid=save-cwd --avoid=select --avoid=sigprocmask --avoid=sys_types --avoid=threadlib --makefile-name=gnulib.mk --conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca-opt c-ctype c-strcase careadlinkat close-stream crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512 dtoastr dtotimespec dup2 environ execinfo faccessat fcntl-h filemode getloadavg getopt-gnu gettime gettimeofday ignore-value intprops largefile lstat manywarnings mktime pselect pthread_sigmask putenv readlink sig2str socklen stat-time stdalign stdarg stdbool stdio strftime strtoimax strtoumax symlink sys_stat sys_time time timer-time timespec-add timespec-sub uns
 etenv uti
mens warnings
+# Reproduce by: gnulib-tool --import --dir=. --lib=libgnu --source-base=lib --m4-base=m4 --doc-base=doc --tests-base=tests --aux-dir=build-aux --avoid=at-internal --avoid=errno --avoid=fchdir --avoid=fcntl --avoid=fstat --avoid=malloc-posix --avoid=msvc-inval --avoid=msvc-nothrow --avoid=openat-die --avoid=openat-h --avoid=raise --avoid=save-cwd --avoid=select --avoid=sigprocmask --avoid=sys_types --avoid=threadlib --makefile-name=gnulib.mk --conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files _Generic alloca-opt c-ctype c-strcase careadlinkat close-stream crypto/md5 crypto/sha1 crypto/sha256 crypto/sha512 dtoastr dtotimespec dup2 environ execinfo faccessat fcntl-h filemode getloadavg getopt-gnu gettime gettimeofday ignore-value imaxabs intprops largefile lstat manywarnings mktime pselect pthread_sigmask putenv readlink sig2str socklen stat-time stdalign stdarg stdbool stdio strftime strtoimax strtoumax symlink sys_stat sys_time time timer-time timespec-add
  timespec
-sub unsetenv utimens warnings
 
 
 MOSTLYCLEANFILES += core *.stackdump
@@ -343,6 +343,15 @@
 
 ## end   gnulib module ignore-value
 
+## begin gnulib module imaxabs
+
+
+EXTRA_DIST += imaxabs.c
+
+EXTRA_libgnu_a_SOURCES += imaxabs.c
+
+## end   gnulib module imaxabs
+
 ## begin gnulib module intprops
 
 

=== added file 'lib/imaxabs.c'
--- lib/imaxabs.c	1970-01-01 00:00:00 +0000
+++ lib/imaxabs.c	2013-01-10 08:23:32 +0000
@@ -0,0 +1,26 @@
+/* imaxabs() function: absolute value of 'intmax_t'.
+   Copyright (C) 2006, 2009-2013 Free Software Foundation, Inc.
+
+   This program 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.
+
+   This program 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 this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <config.h>
+
+/* Specification.  */
+#include <inttypes.h>
+
+intmax_t
+imaxabs (intmax_t x)
+{
+  return (x >= 0 ? x : - x);
+}

=== added file 'm4/_Generic.m4'
--- m4/_Generic.m4	1970-01-01 00:00:00 +0000
+++ m4/_Generic.m4	2013-01-12 01:03:44 +0000
@@ -0,0 +1,34 @@
+# Test for C11-style _Generic
+dnl Copyright 2012-2013 Free Software Foundation, Inc.
+dnl This file is free software; the Free Software Foundation
+dnl gives unlimited permission to copy and/or distribute it,
+dnl with or without modifications, as long as this notice is preserved.
+
+dnl Written by Paul Eggert.
+
+# Backport of autoconf-2.70's macro.
+# Remove this macro when we can assume autoconf >= 2.70.
+m4_ifndef([AC_C__GENERIC],
+[AC_DEFUN([AC_C__GENERIC],
+[AC_CACHE_CHECK([for _Generic], ac_cv_c__Generic,
+[AC_COMPILE_IFELSE(
+   [AC_LANG_SOURCE(
+      [[int
+	 main (int argc, char **argv)
+	 {
+	   int a = _Generic (argc, int: argc = 1);
+	   int *b = &_Generic (argc, default: argc);
+	   char ***c = _Generic (argv, int: argc, default: argv ? &argv : 0);
+	   _Generic (1 ? 0 : b, int: a, default: b) = &argc;
+	   _Generic (a = 1, default: a) = 3;
+	   return a + !b + !c;
+	 }
+      ]])],
+   [ac_cv_c__Generic=yes],
+   [ac_cv_c__Generic=no])])
+if test $ac_cv_c__Generic = yes; then
+  AC_DEFINE([HAVE_C__GENERIC], 1,
+	    [Define to 1 if C11-style _Generic works.])
+fi
+])
+])

=== modified file 'm4/gnulib-comp.m4'
--- m4/gnulib-comp.m4	2013-01-04 02:17:49 +0000
+++ m4/gnulib-comp.m4	2013-01-12 01:03:44 +0000
@@ -38,6 +38,7 @@
   m4_pattern_allow([^gl_LIBOBJS$])dnl a variable
   m4_pattern_allow([^gl_LTLIBOBJS$])dnl a variable
   AC_REQUIRE([gl_PROG_AR_RANLIB])
+  # Code from module _Generic:
   # Code from module alloca-opt:
   # Code from module allocator:
   # Code from module c-ctype:
@@ -72,6 +73,7 @@
   # Code from module gettimeofday:
   # Code from module group-member:
   # Code from module ignore-value:
+  # Code from module imaxabs:
   # Code from module include_next:
   # Code from module intprops:
   # Code from module inttypes-incomplete:
@@ -150,6 +152,7 @@
   m4_pushdef([gl_LIBSOURCES_DIR], [])
   gl_COMMON
   gl_source_base='lib'
+  AC_C__GENERIC
   gl_FUNC_ALLOCA
   AC_CHECK_FUNCS_ONCE([readlinkat])
   gl_CLOCK_TIME
@@ -216,6 +219,12 @@
     gl_PREREQ_GETTIMEOFDAY
   fi
   gl_SYS_TIME_MODULE_INDICATOR([gettimeofday])
+  gl_FUNC_IMAXABS
+  if test $ac_cv_func_imaxabs = no; then
+    AC_LIBOBJ([imaxabs])
+    gl_PREREQ_IMAXABS
+  fi
+  gl_INTTYPES_MODULE_INDICATOR([imaxabs])
   gl_INTTYPES_INCOMPLETE
   AC_REQUIRE([gl_LARGEFILE])
   gl_FUNC_LSTAT
@@ -682,6 +691,7 @@
   lib/gettimeofday.c
   lib/group-member.c
   lib/ignore-value.h
+  lib/imaxabs.c
   lib/intprops.h
   lib/inttypes.in.h
   lib/lstat.c
@@ -742,6 +752,7 @@
   lib/verify.h
   lib/xalloc-oversized.h
   m4/00gnulib.m4
+  m4/_Generic.m4
   m4/alloca.m4
   m4/c-strtod.m4
   m4/clock_time.m4
@@ -764,6 +775,7 @@
   m4/gettimeofday.m4
   m4/gnulib-common.m4
   m4/group-member.m4
+  m4/imaxabs.m4
   m4/include_next.m4
   m4/inttypes.m4
   m4/largefile.m4

=== added file 'm4/imaxabs.m4'
--- m4/imaxabs.m4	1970-01-01 00:00:00 +0000
+++ m4/imaxabs.m4	2013-01-10 08:23:32 +0000
@@ -0,0 +1,20 @@
+# imaxabs.m4 serial 4
+dnl Copyright (C) 2006, 2009-2013 Free Software Foundation, Inc.
+dnl This file is free software; the Free Software Foundation
+dnl gives unlimited permission to copy and/or distribute it,
+dnl with or without modifications, as long as this notice is preserved.
+
+AC_DEFUN([gl_FUNC_IMAXABS],
+[
+  AC_REQUIRE([gl_INTTYPES_H_DEFAULTS])
+
+  dnl On OSF/1 5.1 with cc, this function is declared but not defined.
+  AC_CHECK_FUNCS_ONCE([imaxabs])
+  AC_CHECK_DECLS_ONCE([imaxabs])
+  if test "$ac_cv_have_decl_imaxabs" != yes; then
+    HAVE_DECL_IMAXABS=0
+  fi
+])
+
+# Prerequisites of lib/imaxabs.c.
+AC_DEFUN([gl_PREREQ_IMAXABS], [:])

=== modified file 'nt/ChangeLog'
--- nt/ChangeLog	2013-01-11 09:33:54 +0000
+++ nt/ChangeLog	2013-01-12 01:04:47 +0000
@@ -1,3 +1,8 @@
+2013-01-12  Paul Eggert  <eggert <at> cs.ucla.edu>
+
+	Make eabs act more like a function.
+	* inc/inttypes.h (imaxabs) [!__MINGW32__]: New macro.
+
 2013-01-11  Eli Zaretskii  <eliz <at> gnu.org>
 
 	* inc/unistd.h (O_IGNORE_CTTY): Define, as it is unconditionally

=== modified file 'nt/inc/inttypes.h'
--- nt/inc/inttypes.h	2013-01-01 09:11:05 +0000
+++ nt/inc/inttypes.h	2013-01-11 23:37:49 +0000
@@ -25,9 +25,11 @@
 #else  /* !__MINGW32__ */
 #include "stdint.h"
 #ifdef _WIN64
+#define imaxabs _abs64
 #define strtoumax _strtoui64
 #define strtoimax _strtoi64
 #else
+#define imaxabs labs
 #define strtoumax strtoul
 #define strtoimax strtol
 #endif

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2013-01-12 06:15:12 +0000
+++ src/ChangeLog	2013-01-12 06:31:32 +0000
@@ -1,3 +1,8 @@
+2013-01-12  Paul Eggert  <eggert <at> cs.ucla.edu>
+
+	Make eabs act more like a function.
+	* lisp.h (eabs): Evaluate arg just once.
+
 2013-01-12  Dmitry Antipov  <dmantipov <at> yandex.ru>
 
 	* indent.c (Fvertical_motion): Remove now-incorrect GCPROs

=== modified file 'src/lisp.h'
--- src/lisp.h	2013-01-12 05:21:06 +0000
+++ src/lisp.h	2013-01-12 06:31:32 +0000
@@ -3626,11 +3626,21 @@
 /* Set up the name of the machine we're running on.  */
 extern void init_system_name (void);
 
-/* We used to use `abs', but that clashes with system headers on some
-   platforms, and using a name reserved by Standard C is a bad idea
-   anyway.  */
-#if !defined (eabs)
-#define eabs(x)         ((x) < 0 ? -(x) : (x))
+/* Return the absolute value of X, without evaluating X more than once.
+   X should be a signed integer, and X's absolute value should not
+   exceed the maximum for its promoted type.
+
+   If _Generic or typeof works, speed things up a bit by avoiding the
+   conversion to intmax_t.  The "~" promotes X's type, and checks that
+   X is an integer.  */
+#if HAVE_C__GENERIC
+# define eabs(x) \
+    (_Generic (~ (x), int: abs, long: labs, long long: llabs, default: imaxabs) \
+     (x))
+#elif HAVE_TYPEOF
+# define eabs(x) ({ typeof (~ (x)) eabsx = x; eabsx < 0 ? -eabsx : eabsx; })
+#else
+# define eabs(x) imaxabs (x)
 #endif
 
 /* Return a fixnum or float, depending on whether VAL fits in a Lisp

Message #8 received at 13419 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 13419 <at> debbugs.gnu.org
Subject: Re: bug#13419: make 'eabs' act more like a function
Date: Sat, 12 Jan 2013 11:11:28 +0200

> Date: Fri, 11 Jan 2013 22:44:19 -0800
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> 
> The key part of this patch is to lisp.h; most of the rest
> is pulled in from gnulib automatically.  I'm guessing about
> the change to nt/inc/inttypes.h, as I don't use Windows.

Thanks.  The change to nt/inc/inttypes.h should be OK for the MS
compiler, I think, with one gotcha:

> +#define imaxabs labs

This should be just "abs".  There's no 'labs' in the MS compiler,
according to the MS documentation, and 'long' is 32-bit wide, so 'abs'
is fine.  The _abs64 definition for the 64-bit build is OK, AFAIU.

Given these macros, the Windows build doesn't need lib/imaxabs.c,
right (since MinGW defines imaxabs in its inttypes.h)?  Because if it
does, lib/makefile.w32-in needs a suitable change.

Message #11 received at 13419 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 13419 <at> debbugs.gnu.org
Subject: Re: bug#13419: make 'eabs' act more like a function
Date: Sat, 12 Jan 2013 08:25:06 -0500

> This patch doesn't fix any bugs, it's just a code cleanup.

[...]

> -/* We used to use `abs', but that clashes with system headers on some
> -   platforms, and using a name reserved by Standard C is a bad idea
> -   anyway.  */
> -#if !defined (eabs)
> -#define eabs(x)         ((x) < 0 ? -(x) : (x))
> +/* Return the absolute value of X, without evaluating X more than once.
> +   X should be a signed integer, and X's absolute value should not
> +   exceed the maximum for its promoted type.
> +
> +   If _Generic or typeof works, speed things up a bit by avoiding the
> +   conversion to intmax_t.  The "~" promotes X's type, and checks that
> +   X is an integer.  */
> +#if HAVE_C__GENERIC
> +# define eabs(x) \
> +    (_Generic (~ (x), int: abs, long: labs, long long: llabs, default: imaxabs) \
> +     (x))
> +#elif HAVE_TYPEOF
> +# define eabs(x) ({ typeof (~ (x)) eabsx = x; eabsx < 0 ? -eabsx : eabsx; })
> +#else
> +# define eabs(x) imaxabs (x)
>  #endif
 
Huh?  Do you really consider this code cleaner?  It's hideous and
impenetrable.  Not cool!


        Stefan

Message #14 received at 13419 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 13419 <at> debbugs.gnu.org
Subject: Re: bug#13419: make 'eabs' act more like a function
Date: Sat, 12 Jan 2013 15:06:37 -0800

[Message part 1 (text/plain, inline)]

On 01/12/2013 05:25 AM, Stefan Monnier wrote:
> Do you really consider this code cleaner?

The implementation's worse, but the use is nicer,
since eabs is guaranteed to evaluate its argument just once.

Anyway, here's a simpler implementation that also evaluates
its argument just once: would you prefer this?

  #define eabs(x) imaxabs (x)

(This is shorter and simpler than what's in Emacs now.)

I've attached a complete patch based on this idea.
It incorporates Eli's suggestion to implement imaxabs
via abs rather than labs on 32-bit MS-Windows.  (The
idea is to avoid the need to build imaxabs.o on Windows.)

[eabs-imaxabs.txt (text/plain, attachment)]

Message #17 received at 13419 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 13419 <at> debbugs.gnu.org
Subject: Re: bug#13419: make 'eabs' act more like a function
Date: Sat, 12 Jan 2013 19:26:55 -0500

>> Do you really consider this code cleaner?
> The implementation's worse, but the use is nicer,
> since eabs is guaranteed to evaluate its argument just once.

I understand this, but I think the price is too high.

> Anyway, here's a simpler implementation that also evaluates
> its argument just once: would you prefer this?
>   #define eabs(x) imaxabs (x)

But then it's not inlined, right?
I think this is trying to fix something that's not broken and
C simply does not offer a good enough alternative.


        Stefan

Message #20 received at 13419 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 13419 <at> debbugs.gnu.org
Subject: Re: bug#13419: make 'eabs' act more like a function
Date: Sat, 12 Jan 2013 17:41:11 -0800

On 01/12/2013 04:26 PM, Stefan Monnier wrote:
>>   #define eabs(x) imaxabs (x)
> But then it's not inlined, right?

No, it's inlined, if the compiler's any good.
GCC inlines it, for example.  So it is a cleaner
and efficient replacement for what Emacs has now.

Message #25 received at 13419-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: 13419-done <at> debbugs.gnu.org
Subject: Re: bug#13419: make 'eabs' act more like a function
Date: Sat, 19 Jan 2013 14:36:09 -0800

No further comment, and as this was a low-priority refactoring
that doesn't have consensus I am marking this bug as done without
installing the patch.  I did patch the comment that describes 'eabs',
and removed an unnecessary "#if", as trunk bzr 111566.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.