[PATCH] Clean up a couple of compiler warnings

[PATCH] Clean up a couple of compiler warnings

[Philipp Stephani]

* emacs.c (using_utf8): Don't assume anything about mbstate_t type.

* fileio.c (file_name_case_insensitive_p): Add cast.

* lread.c (string_to_number): Use constants of double type.
* editfns.c (decode_float_time):
* fns.c (make_hash_table, maybe_resize_hash_table)
(Fhash_table_rehash_size, Fhash_table_rehash_threshold):
Explicitly cast floating-point values.

* sysdep.c (system_process_attributes): Remove unused variables.

* emacs-module.c (MODULE_SETJMP_1): Mark dummy variable as unused.
---
 src/editfns.c      |  2 +-
 src/emacs-module.c |  2 +-
 src/emacs.c        |  3 ++-
 src/fileio.c       |  6 +++++-
 src/fns.c          | 10 +++++-----
 src/lread.c        |  4 ++--
 src/sysdep.c       |  5 -----
 7 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/src/editfns.c b/src/editfns.c
index 75eb75a729..8b7854cda5 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -1763,7 +1763,7 @@ decode_float_time (double t, struct lisp_time *result)
   EMACS_INT hi = small_t;
   double t_sans_hi = t - hi * lo_multiplier;
   int lo = t_sans_hi;
-  long double fracps = (t_sans_hi - lo) * 1e12L;
+  long double fracps = (long double) (t_sans_hi - lo) * 1e12L;
 #ifdef INT_FAST64_MAX
   int_fast64_t ifracps = fracps;
   int us = ifracps / 1000000;
diff --git a/src/emacs-module.c b/src/emacs-module.c
index cd025a1396..2f7e37d5be 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -170,7 +170,7 @@ static emacs_value const module_nil = 0;
       return retval;							\
     }									\
   verify (module_has_cleanup);						\
-  int dummy __attribute__ ((cleanup (module_reset_handlerlist)));	\
+  int dummy __attribute__ ((cleanup (module_reset_handlerlist), unused)); \
   if (sys_setjmp (c->jmp))						\
     {									\
       (handlerfunc) (env, c->val);					\
diff --git a/src/emacs.c b/src/emacs.c
index 3aa914f22f..208b4b2a3f 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -347,7 +347,8 @@ using_utf8 (void)
 {
 #ifdef HAVE_WCHAR_H
   wchar_t wc;
-  mbstate_t mbs = { 0 };
+  mbstate_t mbs;
+  memset (&mbs, 0, sizeof mbs);
   return mbrtowc (&wc, "\xc4\x80", 2, &mbs) == 2 && wc == 0x100;
 #else
   return false;
diff --git a/src/fileio.c b/src/fileio.c
index acbf76e0d8..be1a2659b8 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2282,6 +2282,10 @@ file_name_case_insensitive_p (const char *filename)
   int DARWIN_OS_CASE_SENSITIVE_FIXME = 0;
 # endif
 
+  /* FIXME: The next two branches are both incorrect because the
+     attribute buffer starts with a 32-bit integer specifying its
+     size.  This size is not included in the vol_capabilities_attr
+     structure.  */
   if (DARWIN_OS_CASE_SENSITIVE_FIXME == 1)
     {
       /* This is based on developer.apple.com's getattrlist man page.  */
@@ -2306,7 +2310,7 @@ file_name_case_insensitive_p (const char *filename)
       if (getattrlist (filename, &alist, buffer, sizeof (buffer), 0)
 	  || !(alist.volattr & ATTR_VOL_CAPABILITIES))
 	return 0;
-      vol_capabilities_attr_t *vcaps = buffer;
+      vol_capabilities_attr_t *vcaps = (void *) buffer;
       return !(vcaps->capabilities[0] & VOL_CAP_FMT_CASE_SENSITIVE);
     }
 #endif	/* DARWIN_OS */
diff --git a/src/fns.c b/src/fns.c
index 0332ab5dad..bd05b4803e 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -3772,7 +3772,7 @@ make_hash_table (struct hash_table_test test, EMACS_INT size,
   if (size == 0)
     size = 1;
 
-  double threshold = rehash_threshold;
+  double threshold = (double) rehash_threshold;
   index_float = size / threshold;
   index_size = (index_float < INDEX_SIZE_BOUND + 1
 		? next_almost_prime (index_float)
@@ -3854,7 +3854,7 @@ maybe_resize_hash_table (struct Lisp_Hash_Table *h)
       ptrdiff_t old_size = HASH_TABLE_SIZE (h);
       EMACS_INT new_size, index_size, nsize;
       ptrdiff_t i;
-      double rehash_size = h->rehash_size;
+      double rehash_size = (double) h->rehash_size;
       double index_float;
 
       if (rehash_size < 0)
@@ -3869,7 +3869,7 @@ maybe_resize_hash_table (struct Lisp_Hash_Table *h)
 	}
       if (new_size <= old_size)
 	new_size = old_size + 1;
-      double threshold = h->rehash_threshold;
+      double threshold = (double) h->rehash_threshold;
       index_float = new_size / threshold;
       index_size = (index_float < INDEX_SIZE_BOUND + 1
 		    ? next_almost_prime (index_float)
@@ -4564,7 +4564,7 @@ DEFUN ("hash-table-rehash-size", Fhash_table_rehash_size,
        doc: /* Return the current rehash size of TABLE.  */)
   (Lisp_Object table)
 {
-  double rehash_size = check_hash_table (table)->rehash_size;
+  double rehash_size = (double) check_hash_table (table)->rehash_size;
   if (rehash_size < 0)
     {
       EMACS_INT s = -rehash_size;
@@ -4580,7 +4580,7 @@ DEFUN ("hash-table-rehash-threshold", Fhash_table_rehash_threshold,
        doc: /* Return the current rehash threshold of TABLE.  */)
   (Lisp_Object table)
 {
-  return make_float (check_hash_table (table)->rehash_threshold);
+  return make_float ((double) check_hash_table (table)->rehash_threshold);
 }
 
 
diff --git a/src/lread.c b/src/lread.c
index 5e737d690c..87cac3cc25 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -3568,7 +3568,7 @@ string_to_number (char const *string, int base, bool ignore_trailing)
 	    {
 	      state |= E_EXP;
 	      cp += 3;
-	      value = INFINITY;
+	      value = HUGE_VAL;
 	    }
 	  else if (cp[-1] == '+'
 		   && cp[0] == 'N' && cp[1] == 'a' && cp[2] == 'N')
@@ -3576,7 +3576,7 @@ string_to_number (char const *string, int base, bool ignore_trailing)
 	      state |= E_EXP;
 	      cp += 3;
 	      /* NAN is a "positive" NaN on all known Emacs hosts.  */
-	      value = NAN;
+	      value = nan(NULL);
 	    }
 	  else
 	    cp = ecp;
diff --git a/src/sysdep.c b/src/sysdep.c
index 91b2a5cb94..ac6eed0e58 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -3707,14 +3707,9 @@ Lisp_Object
 system_process_attributes (Lisp_Object pid)
 {
   int proc_id;
-  int pagesize = getpagesize ();
-  unsigned long npages;
-  int fscale;
   struct passwd *pw;
   struct group  *gr;
   char *ttyname;
-  size_t len;
-  char args[MAXPATHLEN];
   struct timeval starttime;
   struct timespec t, now;
   struct rusage *rusage;
-- 
2.13.0

Re: [PATCH] Clean up a couple of compiler warnings

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Thu, 18 May 2017 22:24:50 +0200
> Cc: Philipp Stephani <phst@google.com>
> 
> * emacs.c (using_utf8): Don't assume anything about mbstate_t type.
> 
> * fileio.c (file_name_case_insensitive_p): Add cast.
> 
> * lread.c (string_to_number): Use constants of double type.
> * editfns.c (decode_float_time):
> * fns.c (make_hash_table, maybe_resize_hash_table)
> (Fhash_table_rehash_size, Fhash_table_rehash_threshold):
> Explicitly cast floating-point values.
> 
> * sysdep.c (system_process_attributes): Remove unused variables.
> 
> * emacs-module.c (MODULE_SETJMP_1): Mark dummy variable as unused.

Could you please show all the warnings which triggered these changes?
Some of the changes look very strange to me, so I'd like to see the
warnings first.

Thanks.

Re: [PATCH] Clean up a couple of compiler warnings

[Paul Eggert]

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

Philipp Stephani wrote:

 > * sysdep.c (system_process_attributes): Remove unused variables.

Thanks, I installed that in your name (1st attached patch).


> * emacs.c (using_utf8): Don't assume anything about mbstate_t type.

The old code doesn't assume anything about the type either, and should work 
regardless of how mbstate_t is implemented. The proposed change makes the code a 
bit harder to read, so I'd rather avoid it if possible. I couldn't reproduce the 
warning that you're evidently seeing. Perhaps you can work around the problem in 
the same way I worked around the problem with floats and doubles (see below)?


> * emacs-module.c (MODULE_SETJMP_1): Mark dummy variable as unused.

Rather than do that, let's just use the variable; that's more robust. I 
installed the 2nd attached patch.

> * lread.c (string_to_number): Use constants of double type.
> * editfns.c (decode_float_time):
> * fns.c (make_hash_table, maybe_resize_hash_table)
> (Fhash_table_rehash_size, Fhash_table_rehash_threshold):
> Explicitly cast floating-point values.

There should be no problem (i.e., no loss of info) converting a float to a 
double, so these changes are not needed. I assume you're using clang. I 
reproduced its false alarms on Fedora 25 x86-64, which uses clang 3.9.1, and 
installed the 3rd attached patch to work around the problem.


 > * fileio.c (file_name_case_insensitive_p): Add cast.

Rather than waste time static-checking the DARWIN_OS_CASE_SENSITIVE_FIXME == 2 
code let's just #ifdef it out. I did that in the 4th attached patch. Maybe we 
should just remove it, since nobody is using it and (as you note) it doesn't 
work anyway.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Clean-up-some-compiler-warnings.patch --]
[-- Type: text/x-patch; name="0001-Clean-up-some-compiler-warnings.patch", Size: 854 bytes --]

From e41030971f37375b9bb16c248f3b5d8d12064743 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Thu, 18 May 2017 19:15:26 -0700
Subject: [PATCH 1/4] Clean up some compiler warnings

* src/sysdep.c (system_process_attributes) [DARWIN_OS]:
Remove unused locals.
---
 src/sysdep.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/src/sysdep.c b/src/sysdep.c
index 91b2a5c..ac6eed0 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -3707,14 +3707,9 @@ Lisp_Object
 system_process_attributes (Lisp_Object pid)
 {
   int proc_id;
-  int pagesize = getpagesize ();
-  unsigned long npages;
-  int fscale;
   struct passwd *pw;
   struct group  *gr;
   char *ttyname;
-  size_t len;
-  char args[MAXPATHLEN];
   struct timeval starttime;
   struct timespec t, now;
   struct rusage *rusage;
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Clean-up-compiler-warning-in-emacs-module.c.patch --]
[-- Type: text/x-patch; name="0002-Clean-up-compiler-warning-in-emacs-module.c.patch", Size: 2626 bytes --]

From 9647430ae8744bd4dee29f1269b74f65c09274d2 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 18 May 2017 20:02:42 -0700
Subject: [PATCH 2/4] Clean up compiler warning in emacs-module.c

* src/emacs-module.c (MODULE_SETJMP_1): Use the local var
instead of leaving it unused, to pacify picky compilers.
(module_reset_handlerlist): Now takes a dummy pointer to a struct
handler *, instead of a dummy pointer to an int.  All uses changed.
---
 src/emacs-module.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/emacs-module.c b/src/emacs-module.c
index cd025a1..5aa8a88 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -109,7 +109,7 @@ static void module_handle_throw (emacs_env *, Lisp_Object);
 static void module_non_local_exit_signal_1 (emacs_env *, Lisp_Object, Lisp_Object);
 static void module_non_local_exit_throw_1 (emacs_env *, Lisp_Object, Lisp_Object);
 static void module_out_of_memory (emacs_env *);
-static void module_reset_handlerlist (const int *);
+static void module_reset_handlerlist (struct handler *const *);
 
 /* We used to return NULL when emacs_value was a different type from
    Lisp_Object, but nowadays we just use Qnil instead.  Although they
@@ -160,17 +160,18 @@ static emacs_value const module_nil = 0;
 
 /* TODO: Make backtraces work if this macros is used.  */
 
-#define MODULE_SETJMP_1(handlertype, handlerfunc, retval, c, dummy)	\
+#define MODULE_SETJMP_1(handlertype, handlerfunc, retval, c0, c)	\
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)	\
     return retval;							\
-  struct handler *c = push_handler_nosignal (Qt, handlertype);		\
-  if (!c)								\
+  struct handler *c0 = push_handler_nosignal (Qt, handlertype);		\
+  if (!c0)								\
     {									\
       module_out_of_memory (env);					\
       return retval;							\
     }									\
   verify (module_has_cleanup);						\
-  int dummy __attribute__ ((cleanup (module_reset_handlerlist)));	\
+  struct handler *c __attribute__ ((cleanup (module_reset_handlerlist))) \
+    = c0;								\
   if (sys_setjmp (c->jmp))						\
     {									\
       (handlerfunc) (env, c->val);					\
@@ -919,7 +920,7 @@ finalize_environment (struct emacs_env_private *env)
    code in eval.c for details.  The macros below arrange for this
    function to be called automatically.  DUMMY is ignored.  */
 static void
-module_reset_handlerlist (const int *dummy)
+module_reset_handlerlist (struct handler *const *dummy)
 {
   handlerlist = handlerlist->next;
 }
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Port-enable-gcc-warnings-to-clang-3.9.1.patch --]
[-- Type: text/x-patch; name="0003-Port-enable-gcc-warnings-to-clang-3.9.1.patch", Size: 780 bytes --]

From af4ac3dc852647e5ea41be5ffc387be591978aee Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 18 May 2017 20:27:11 -0700
Subject: [PATCH 3/4] Port --enable-gcc-warnings to clang 3.9.1

* configure.ac (WERROR_CFLAGS): Omit -Wdouble-promotion if clang.
Problem reported by Philipp Stephani in:
http://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00495.html
---
 configure.ac | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure.ac b/configure.ac
index 34b75a7..03542a6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -963,6 +963,7 @@ AC_DEFUN
 
   if test $emacs_cv_clang = yes; then
     nw="$nw -Wcast-align"
+    nw="$nw -Wdouble-promotion"
   fi
 
   # This causes too much noise in the MinGW build
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Fix-DARWIN_OS_CASE_SENSITIVE_FIXME-2-false-alarm.patch --]
[-- Type: text/x-patch; name="0004-Fix-DARWIN_OS_CASE_SENSITIVE_FIXME-2-false-alarm.patch", Size: 1784 bytes --]

From 7e3e51f4e288379be6a8731b739b63b9386940f7 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 18 May 2017 20:40:42 -0700
Subject: [PATCH 4/4] Fix DARWIN_OS_CASE_SENSITIVE_FIXME==2 false alarm
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/fileio.c (file_name_case_insensitive_p):
Don’t compile the (DARWIN_OS_CASE_SENSITIVE_FIXME == 2)
code unless DARWIN_OS_CASE_SENSITIVE_FIXME is 2.
Problem reported by Philipp Stephani in:
http://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00495.html
---
 src/fileio.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/fileio.c b/src/fileio.c
index acbf76e..e5e3505 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2294,10 +2294,14 @@ file_name_case_insensitive_p (const char *filename)
 		      & VOL_CAP_FMT_CASE_SENSITIVE);
 	}
     }
-  else if (DARWIN_OS_CASE_SENSITIVE_FIXME == 2)
+# if DARWIN_OS_CASE_SENSITIVE_FIXME == 2
     {
       /* The following is based on
-	 http://lists.apple.com/archives/darwin-dev/2007/Apr/msg00010.html.  */
+	 http://lists.apple.com/archives/darwin-dev/2007/Apr/msg00010.html.
+	 It is normally not even compiled, since it runs afoul of
+	 static checking.  See:
+	 http://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00495.html
+         */
       struct attrlist alist;
       unsigned char buffer[sizeof (vol_capabilities_attr_t) + sizeof (size_t)];
 
@@ -2309,6 +2313,7 @@ file_name_case_insensitive_p (const char *filename)
       vol_capabilities_attr_t *vcaps = buffer;
       return !(vcaps->capabilities[0] & VOL_CAP_FMT_CASE_SENSITIVE);
     }
+# endif
 #endif	/* DARWIN_OS */
 
 #if defined CYGWIN || defined DOS_NT
-- 
2.7.4

Re: [PATCH] Clean up a couple of compiler warnings

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 18 May 2017 20:48:42 -0700
> Cc: Philipp Stephani <phst@google.com>
> 
>  > * fileio.c (file_name_case_insensitive_p): Add cast.
> 
> Rather than waste time static-checking the DARWIN_OS_CASE_SENSITIVE_FIXME == 2 
> code let's just #ifdef it out. I did that in the 4th attached patch. Maybe we 
> should just remove it, since nobody is using it and (as you note) it doesn't 
> work anyway.
> [...]
> diff --git a/src/fileio.c b/src/fileio.c
> index acbf76e..e5e3505 100644
> --- a/src/fileio.c
> +++ b/src/fileio.c
> @@ -2294,10 +2294,14 @@ file_name_case_insensitive_p (const char *filename)
>  		      & VOL_CAP_FMT_CASE_SENSITIVE);
>  	}
>      }
> -  else if (DARWIN_OS_CASE_SENSITIVE_FIXME == 2)
> +# if DARWIN_OS_CASE_SENSITIVE_FIXME == 2
>      {
>        /* The following is based on
> -	 http://lists.apple.com/archives/darwin-dev/2007/Apr/msg00010.html.  */
> +	 http://lists.apple.com/archives/darwin-dev/2007/Apr/msg00010.html.
> +	 It is normally not even compiled, since it runs afoul of
> +	 static checking.  See:
> +	 http://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00495.html
> +         */
>        struct attrlist alist;
>        unsigned char buffer[sizeof (vol_capabilities_attr_t) + sizeof (size_t)];
>  
> @@ -2309,6 +2313,7 @@ file_name_case_insensitive_p (const char *filename)
>        vol_capabilities_attr_t *vcaps = buffer;
>        return !(vcaps->capabilities[0] & VOL_CAP_FMT_CASE_SENSITIVE);
>      }
> +# endif
>  #endif	/* DARWIN_OS */

Should we make the DARWIN_OS_CASE_SENSITIVE_FIXME == 1 case an ifdef
as well?  It feels strange to try to avoid the ifdef for one value,
but not for the other.

Thanks for this and the other changes.

Re: [PATCH] Clean up a couple of compiler warnings

[Paul Eggert]

Eli Zaretskii wrote:
> Should we make the DARWIN_OS_CASE_SENSITIVE_FIXME == 1 case an ifdef
> as well?

I suppose we could. I'm becoming inclined, though, to remove both the 
DARWIN_OS_CASE_SENSITIVE_FIXME variants, as they're not being used and they're 
consuming scarce maintenance resources.

Re: [PATCH] Clean up a couple of compiler warnings

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am Fr., 19. Mai 2017 um 05:48 Uhr:

> > * emacs.c (using_utf8): Don't assume anything about mbstate_t type.
>
> The old code doesn't assume anything about the type either, and should work
> regardless of how mbstate_t is implemented. The proposed change makes the
> code a
> bit harder to read, so I'd rather avoid it if possible. I couldn't
> reproduce the
> warning that you're evidently seeing.
>

Apparently on some systems mbstate_t is a nested struct, and the compiler
warns about missing braces. Note that memset to initialize a mbstate_t is
explicitly recommended in the libc manual:
https://www.gnu.org/software/libc/manual/html_node/Keeping-the-state.html


>
>
> > * emacs-module.c (MODULE_SETJMP_1): Mark dummy variable as unused.
>
> Rather than do that, let's just use the variable; that's more robust. I
> installed the 2nd attached patch.
>

OK. If you want, you could now add
eassert (handlerlist == *dummy);
or so to the cleanup function.


>
> > * lread.c (string_to_number): Use constants of double type.
> > * editfns.c (decode_float_time):
> > * fns.c (make_hash_table, maybe_resize_hash_table)
> > (Fhash_table_rehash_size, Fhash_table_rehash_threshold):
> > Explicitly cast floating-point values.
>
> There should be no problem (i.e., no loss of info) converting a float to a
> double, so these changes are not needed. I assume you're using clang. I
> reproduced its false alarms on Fedora 25 x86-64, which uses clang 3.9.1,
> and
> installed the 3rd attached patch to work around the problem.
>

Looks good. I'm no fan of silencing implicit conversion warnings by
introducing explicit casts either.


>
>
>  > * fileio.c (file_name_case_insensitive_p): Add cast.
>
> Rather than waste time static-checking the DARWIN_OS_CASE_SENSITIVE_FIXME
> == 2
> code let's just #ifdef it out. I did that in the 4th attached patch. Maybe
> we
> should just remove it, since nobody is using it and (as you note) it
> doesn't
> work anyway.
>

I think none of the four branches there work for macOS.
https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man2/pathconf.2.html
is
silent about case sensitivity, so we're already relying on some
undocumented functionality. The getattrlist method is at least documented (
https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man2/getattrlist.2.html),
it just needs to be implemented correctly. I'd suggest to only use a
working version of getattrlist on macOS.
-- 

Google Germany GmbH
Erika-Mann-Straße 33
80636 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle

Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den Absender und
löschen Sie die E-Mail und alle Anhänge. Vielen Dank.

This e-mail is confidential. If you are not the right addressee please do
not forward it, please inform the sender, and please erase this e-mail
including any attachments. Thanks.

[-- Attachment #2: Type: text/html, Size: 4539 bytes --]

Re: [PATCH] Clean up a couple of compiler warnings

[Paul Eggert]

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

On 05/19/2017 02:31 AM, Philipp Stephani wrote:
> Apparently on some systems mbstate_t is a nested struct, and the compiler 
> warns about missing braces. Note that memset to initialize a mbstate_t is 
> explicitly recommended in the libc manual:

Yes, of course memset works (which is all that the glibc manual really says). 
It's just that it's verbose and the verbosity isn't needed.

I guess the problem here is that clang's -Wmissing-braces option generates false 
alarms. The Clang folks may fix that someday. In the meantime I installed the 
first attached patch, to turn off that option for Clang.

> you could now add
> eassert (handlerlist == *dummy);
> or so to the cleanup function.

Thanks, good idea, installed in the 2nd attached patch.

> I think none of the four branches there work for macOS. 
> https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man2/pathconf.2.html is 
> silent about case sensitivity, so we're already relying on some undocumented 
> functionality. The getattrlist method is at least documented 
> (https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man2/getattrlist.2.html), 
> it just needs to be implemented correctly. I'd suggest to only use a working 
> version of getattrlist on macOS.

I installed that in the 3rd attached patch, at least I hope it works. But better 
yet, let's drop the getattrlist stuff entirely, as it doesn't seem to be needed 
and is just wasting our maintenance time. I installed that in the 4th attached 
patch.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Port-enable-gcc-warnings-to-clang-3.9.1.patch --]
[-- Type: text/x-patch; name="0001-Port-enable-gcc-warnings-to-clang-3.9.1.patch", Size: 835 bytes --]

From a5acb3701a7b4ab8b82aede308d28a47a383ae9c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 19 May 2017 16:05:31 -0700
Subject: [PATCH 1/2] Port --enable-gcc-warnings to clang 3.9.1

* configure.ac (WERROR_CFLAGS): Omit -Wmissing-braces for Clang,
to shut off a false alarm.  Problem reportd by Philipp Stephani in:
http://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00521.html
---
 configure.ac | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure.ac b/configure.ac
index 03542a6..12e44d9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -964,6 +964,7 @@ AC_DEFUN
   if test $emacs_cv_clang = yes; then
     nw="$nw -Wcast-align"
     nw="$nw -Wdouble-promotion"
+    nw="$nw -Wmissing-braces"
   fi
 
   # This causes too much noise in the MinGW build
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-handlerlist-assertion-to-module-code.patch --]
[-- Type: text/x-patch; name="0002-Add-handlerlist-assertion-to-module-code.patch", Size: 1336 bytes --]

From 7d00410af69b3cbbf0e8fc9765f3bf9f5616286d Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 19 May 2017 16:15:07 -0700
Subject: [PATCH 2/2] Add handlerlist assertion to module code

* src/emacs-module.c (module_reset_handlerlist):
Check handlerlist.  Suggested by Philipp Stephani in:
http://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00521.html
---
 src/emacs-module.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/emacs-module.c b/src/emacs-module.c
index 5aa8a88..0bc1b6c 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -918,10 +918,12 @@ finalize_environment (struct emacs_env_private *env)
 /* Must be called after setting up a handler immediately before
    returning from the function.  See the comments in lisp.h and the
    code in eval.c for details.  The macros below arrange for this
-   function to be called automatically.  DUMMY is ignored.  */
+   function to be called automatically.  PHANDLERLIST points to a word
+   containing the handler list, for sanity checking.  */
 static void
-module_reset_handlerlist (struct handler *const *dummy)
+module_reset_handlerlist (struct handler *const *phandlerlist)
 {
+  eassert (handlerlist == *phandlerlist);
   handlerlist = handlerlist->next;
 }
 
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Narrow-DARWIN_OS_CASE_SENSITIVE_FIXME-to-1-choice.patch --]
[-- Type: text/x-patch; name="0003-Narrow-DARWIN_OS_CASE_SENSITIVE_FIXME-to-1-choice.patch", Size: 5240 bytes --]

From 075bd64609446e741a6efbcd6cd6e232db8d1df6 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 20 May 2017 22:51:32 -0700
Subject: [PATCH 1/2] Narrow DARWIN_OS_CASE_SENSITIVE_FIXME to 1 choice

* etc/PROBLEMS: Document this (Bug#24441).
* src/fileio.c (file_name_case_insensitive_p): Prefer pathconf
with _PC_CASE_SENSITIVE, if it works, to
DARWIN_OS_CASE_SENSITIVE_FIXME code.
Support just one method for DARWIN_OS_CASE_SENSITIVE_FIXME,
which matches the Apple documentation more precisely.
---
 etc/PROBLEMS |  5 ++---
 src/fileio.c | 68 ++++++++++++++++++++----------------------------------------
 2 files changed, 24 insertions(+), 49 deletions(-)

diff --git a/etc/PROBLEMS b/etc/PROBLEMS
index e415887..ff88aa3 100644
--- a/etc/PROBLEMS
+++ b/etc/PROBLEMS
@@ -2486,9 +2486,8 @@ If you do, please send it to bug-gnu-emacs@gnu.org so we can list it here.
 The implementation of that function on Mac OS X uses pathconf with the
 _PC_CASE_SENSITIVE flag.  There have been reports that this use of
 pathconf does not work reliably.  If you have a problem, please
-recompile Emacs with -D DARWIN_OS_CASE_SENSITIVE_FIXME=1 or
--D DARWIN_OS_CASE_SENSITIVE_FIXME=2, and file a bug report saying
-whether this fixed your problem.
+recompile Emacs with -D DARWIN_OS_CASE_SENSITIVE_FIXME, and file a bug
+report saying whether this fixed your problem.
 
 * Build-time problems
 
diff --git a/src/fileio.c b/src/fileio.c
index e5e3505..17659b6 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2256,65 +2256,41 @@ static bool
 file_name_case_insensitive_p (const char *filename)
 {
   /* Use pathconf with _PC_CASE_INSENSITIVE or _PC_CASE_SENSITIVE if
-     those flags are available.  As of this writing (2016-11-14),
+     those flags are available.  As of this writing (2017-05-20),
      Cygwin is the only platform known to support the former (starting
-     with Cygwin-2.6.1), and Mac OS X is the only platform known to
-     support the latter.
-
-     There have been reports that pathconf with _PC_CASE_SENSITIVE
-     does not work reliably on Mac OS X.  If you have a problem,
-     please recompile Emacs with -D DARWIN_OS_CASE_SENSITIVE_FIXME=1 or
-     -D DARWIN_OS_CASE_SENSITIVE_FIXME=2, and file a bug report saying
-     whether this fixed your problem.  */
+     with Cygwin-2.6.1), and macOS is the only platform known to
+     support the latter.  */
 
 #ifdef _PC_CASE_INSENSITIVE
   int res = pathconf (filename, _PC_CASE_INSENSITIVE);
   if (res >= 0)
     return res > 0;
-#elif defined _PC_CASE_SENSITIVE && !defined DARWIN_OS_CASE_SENSITIVE_FIXME
+#elif defined _PC_CASE_SENSITIVE
   int res = pathconf (filename, _PC_CASE_SENSITIVE);
   if (res >= 0)
     return res == 0;
 #endif
 
-#ifdef DARWIN_OS
-# ifndef DARWIN_OS_CASE_SENSITIVE_FIXME
-  int DARWIN_OS_CASE_SENSITIVE_FIXME = 0;
-# endif
+  /* There have been reports that pathconf with _PC_CASE_SENSITIVE
+     does not work reliably on Mac OS X.  If you have a problem,
+     please recompile Emacs with -D DARWIN_OS_CASE_SENSITIVE_FIXME=1 or
+     -D DARWIN_OS_CASE_SENSITIVE_FIXME=2, and file a bug report saying
+     whether this fixed your problem.  */
 
-  if (DARWIN_OS_CASE_SENSITIVE_FIXME == 1)
-    {
-      /* This is based on developer.apple.com's getattrlist man page.  */
-      struct attrlist alist = {.volattr = ATTR_VOL_CAPABILITIES};
-      vol_capabilities_attr_t vcaps;
-      if (getattrlist (filename, &alist, &vcaps, sizeof vcaps, 0) == 0)
-	{
-	  if (vcaps.valid[VOL_CAPABILITIES_FORMAT] & VOL_CAP_FMT_CASE_SENSITIVE)
-	    return ! (vcaps.capabilities[VOL_CAPABILITIES_FORMAT]
-		      & VOL_CAP_FMT_CASE_SENSITIVE);
-	}
-    }
-# if DARWIN_OS_CASE_SENSITIVE_FIXME == 2
-    {
-      /* The following is based on
-	 http://lists.apple.com/archives/darwin-dev/2007/Apr/msg00010.html.
-	 It is normally not even compiled, since it runs afoul of
-	 static checking.  See:
-	 http://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00495.html
-         */
-      struct attrlist alist;
-      unsigned char buffer[sizeof (vol_capabilities_attr_t) + sizeof (size_t)];
-
-      memset (&alist, 0, sizeof (alist));
-      alist.volattr = ATTR_VOL_CAPABILITIES;
-      if (getattrlist (filename, &alist, buffer, sizeof (buffer), 0)
-	  || !(alist.volattr & ATTR_VOL_CAPABILITIES))
-	return 0;
-      vol_capabilities_attr_t *vcaps = buffer;
-      return !(vcaps->capabilities[0] & VOL_CAP_FMT_CASE_SENSITIVE);
-    }
+#ifdef DARWIN_OS_CASE_SENSITIVE_FIXME
+# ifdef VOL_CAP_FMT_CASE_SENSITIVE
+  {
+    struct attrlist alist = {.bitmapcount = ATTR_BIT_MAP_COUNT,
+			     .volattr = ATTR_VOL_INFO | ATTR_VOL_CAPABILITIES};
+    struct { uint32_t len; vol_capabilities_attr_t caps; } vcaps
+      __attribute__ ((aligned (4), packed));
+    int i = VOL_CAPABILITIES_FORMAT;
+    if (getattrlist (filename, &alist, &vcaps, sizeof vcaps, 0) == 0
+	&& (vcaps.caps.valid[i] & VOL_CAP_FMT_CASE_SENSITIVE))
+      return ! (vcaps.caps.capabilities[i] & VOL_CAP_FMT_CASE_SENSITIVE);
+  }
 # endif
-#endif	/* DARWIN_OS */
+#endif
 
 #if defined CYGWIN || defined DOS_NT
   return true;
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Remove-DARWIN_OS_CASE_SENSITIVE_FIXME-code.patch --]
[-- Type: text/x-patch; name="0004-Remove-DARWIN_OS_CASE_SENSITIVE_FIXME-code.patch", Size: 2542 bytes --]

From b35293dfd0e9dd95a88ac01051655d0d2d105992 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 20 May 2017 22:55:17 -0700
Subject: [PATCH 2/2] Remove DARWIN_OS_CASE_SENSITIVE_FIXME code

It does not appear to be needed (Bug#24441).
* etc/PROBLEMS: Remove DARWIN_OS_CASE_SENSITIVE_FIXME stuff.
* src/fileio.c (file_name_case_insensitive_p):
Remove DARWIN_OS_CASE_SENSITIVE_FIXME code.
---
 etc/PROBLEMS | 10 ----------
 src/fileio.c | 21 ---------------------
 2 files changed, 31 deletions(-)

diff --git a/etc/PROBLEMS b/etc/PROBLEMS
index ff88aa3..593eb6b 100644
--- a/etc/PROBLEMS
+++ b/etc/PROBLEMS
@@ -2479,16 +2479,6 @@ please call support for your X-server and see if you can get a fix.
 If you do, please send it to bug-gnu-emacs@gnu.org so we can list it here.
 
 
-* Runtime problems specific to Mac OS X
-
-** On Mac OS X, file-name-case-insensitive-p may be unreliable
-
-The implementation of that function on Mac OS X uses pathconf with the
-_PC_CASE_SENSITIVE flag.  There have been reports that this use of
-pathconf does not work reliably.  If you have a problem, please
-recompile Emacs with -D DARWIN_OS_CASE_SENSITIVE_FIXME, and file a bug
-report saying whether this fixed your problem.
-
 * Build-time problems
 
 ** Configuration
diff --git a/src/fileio.c b/src/fileio.c
index 17659b6..c21056e 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2271,27 +2271,6 @@ file_name_case_insensitive_p (const char *filename)
     return res == 0;
 #endif
 
-  /* There have been reports that pathconf with _PC_CASE_SENSITIVE
-     does not work reliably on Mac OS X.  If you have a problem,
-     please recompile Emacs with -D DARWIN_OS_CASE_SENSITIVE_FIXME=1 or
-     -D DARWIN_OS_CASE_SENSITIVE_FIXME=2, and file a bug report saying
-     whether this fixed your problem.  */
-
-#ifdef DARWIN_OS_CASE_SENSITIVE_FIXME
-# ifdef VOL_CAP_FMT_CASE_SENSITIVE
-  {
-    struct attrlist alist = {.bitmapcount = ATTR_BIT_MAP_COUNT,
-			     .volattr = ATTR_VOL_INFO | ATTR_VOL_CAPABILITIES};
-    struct { uint32_t len; vol_capabilities_attr_t caps; } vcaps
-      __attribute__ ((aligned (4), packed));
-    int i = VOL_CAPABILITIES_FORMAT;
-    if (getattrlist (filename, &alist, &vcaps, sizeof vcaps, 0) == 0
-	&& (vcaps.caps.valid[i] & VOL_CAP_FMT_CASE_SENSITIVE))
-      return ! (vcaps.caps.capabilities[i] & VOL_CAP_FMT_CASE_SENSITIVE);
-  }
-# endif
-#endif
-
 #if defined CYGWIN || defined DOS_NT
   return true;
 #else
-- 
2.7.4

Re: [PATCH] Clean up a couple of compiler warnings

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am So., 21. Mai 2017 um 08:07 Uhr:

> On 05/19/2017 02:31 AM, Philipp Stephani wrote:
> > Apparently on some systems mbstate_t is a nested struct, and the compiler
> > warns about missing braces. Note that memset to initialize a mbstate_t is
> > explicitly recommended in the libc manual:
>
> Yes, of course memset works (which is all that the glibc manual really
> says).
> It's just that it's verbose and the verbosity isn't needed.
>
> I guess the problem here is that clang's -Wmissing-braces option generates
> false
> alarms. The Clang folks may fix that someday. In the meantime I installed
> the
> first attached patch, to turn off that option for Clang.
>

This apparently hasn't worked, at least I still get the warning, and
there's no -Wno-missing-braces in WARN_CFLAGS.
For Clang, it'd probably be better to use -Weverything and then disable
individual warnings with -Wno... instead of checking for the existence of
every warning flag. That would make configure runs faster and the command
line shorter.

[-- Attachment #2: Type: text/html, Size: 1400 bytes --]

Re: [PATCH] Clean up a couple of compiler warnings

[Paul Eggert]

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

Philipp Stephani wrote:
> This apparently hasn't worked, at least I still get the warning, and
> there's no -Wno-missing-braces in WARN_CFLAGS.

My change didn't add -Wno-missing-braces; it removed -Wmissing-braces. This 
sufficed for Fedora 25 x86-64, which has clang 3.9.1.

Perhaps you're running a different version of Clang, which is pickier? If so, 
you might try the attached patch; if it works, please feel free to install it.

> For Clang, it'd probably be better to use -Weverything and then disable
> individual warnings with -Wno

I'd rather not spend a lot of time worrying about warnings generated by 
compilers other than recent GCC, as there are too many compilers and too many 
false alarms and it's not worth the hassle. If you'd like to take up the burden 
of pacifying Clang with tricks like the above, though, that should be fine, as 
long as it doesn't burden maintenance for the rest of us. If not, and if Clang 
continues to be a hassle, we can avoid much of the hassle by disabling warnings 
by default when the compiler is Clang.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: clang.diff --]
[-- Type: text/x-patch; name="clang.diff", Size: 402 bytes --]

diff --git a/configure.ac b/configure.ac
index 12e44d9..6d23b5d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -986,6 +986,7 @@ AC_DEFUN
 
   # More things that clang is unduly picky about.
   if test $emacs_cv_clang = yes; then
+    gl_WARN_ADD([-Wno-missing-braces])
     gl_WARN_ADD([-Wno-tautological-compare])
     gl_WARN_ADD([-Wno-tautological-constant-out-of-range-compare])
   fi

Re: [PATCH] Clean up a couple of compiler warnings

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am So., 21. Mai 2017 um 22:33 Uhr:

> Philipp Stephani wrote:
> > This apparently hasn't worked, at least I still get the warning, and
> > there's no -Wno-missing-braces in WARN_CFLAGS.
>
> My change didn't add -Wno-missing-braces; it removed -Wmissing-braces. This
> sufficed for Fedora 25 x86-64, which has clang 3.9.1.
>
> Perhaps you're running a different version of Clang, which is pickier? If
> so,
> you might try the attached patch; if it works, please feel free to install
> it.
>

I haven't tested it, but I'm pretty sure that adding -Wno-missing-braces
should work.
The warnings enabled for -Wall and -Wextra change from version to version,
so only leaving out -W... will often not be enough, it would be more
future-proof to explicitly specify -Wno... in all cases.


>
> > For Clang, it'd probably be better to use -Weverything and then disable
> > individual warnings with -Wno
>
> I'd rather not spend a lot of time worrying about warnings generated by
> compilers other than recent GCC, as there are too many compilers and too
> many
> false alarms and it's not worth the hassle.


But in practice, at least on Unix-like systems, only GCC and Clang matter,
and Clang is the primary and often the only compiler on systems such as
FreeBSD and macOS.


> If you'd like to take up the burden
> of pacifying Clang with tricks like the above, though, that should be
> fine, as
> long as it doesn't burden maintenance for the rest of us. If not, and if
> Clang
> continues to be a hassle, we can avoid much of the hassle by disabling
> warnings
> by default when the compiler is Clang.
>

It's not a trick. If you don't want the warning FOO to be shown, pass
-Wno-FOO. That should work consistently for both compilers and all
versions.
-- 

Google Germany GmbH
Erika-Mann-Straße 33
80636 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle

Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den Absender und
löschen Sie die E-Mail und alle Anhänge. Vielen Dank.

This e-mail is confidential. If you are not the right addressee please do
not forward it, please inform the sender, and please erase this e-mail
including any attachments. Thanks.

[-- Attachment #2: Type: text/html, Size: 3242 bytes --]

Re: [PATCH] Clean up a couple of compiler warnings

[Paul Eggert]

On 05/23/2017 03:17 AM, Philipp Stephani wrote:
> I haven't tested it, but I'm pretty sure that adding 
> -Wno-missing-braces should work.

I'd feel more comfortable if you tested it and then committed it to master.

> it would be more future-proof to explicitly specify -Wno...

In my experience it's a waste of time to try to future-proof or to 
retrofit GCC's warnings, or Clang's for that matter. The warnings are 
too flaky and are too mutable from one version to the next. At best we 
can arrange for recent GCC to not warn on a few common configurations. 
Maybe Clang too, if someone takes the time to do that.

> But in practice, at least on Unix-like systems, only GCC and Clang matter

Other compilers still matter, I'm afraid. E.g., see 
<http://bugs.gnu.org/26735>, which is currently blocking Emacs release, 
and which is about Intel's C compiler.

Re: [PATCH] Clean up a couple of compiler warnings

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 23 May 2017 10:36:15 -0700
> 
> > it would be more future-proof to explicitly specify -Wno...
> 
> In my experience it's a waste of time to try to future-proof or to 
> retrofit GCC's warnings, or Clang's for that matter. The warnings are 
> too flaky and are too mutable from one version to the next. At best we 
> can arrange for recent GCC to not warn on a few common configurations. 

FWIW, this is my experience as well.

Re: [PATCH] Clean up a couple of compiler warnings

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am Di., 23. Mai 2017 um 19:36 Uhr:

> On 05/23/2017 03:17 AM, Philipp Stephani wrote:
> > I haven't tested it, but I'm pretty sure that adding
> > -Wno-missing-braces should work.
>
> I'd feel more comfortable if you tested it and then committed it to master.
>

Done (cd9c7a0617).


>
> > it would be more future-proof to explicitly specify -Wno...
>
> In my experience it's a waste of time to try to future-proof or to
> retrofit GCC's warnings, or Clang's for that matter. The warnings are
> too flaky and are too mutable from one version to the next. At best we
> can arrange for recent GCC to not warn on a few common configurations.
> Maybe Clang too, if someone takes the time to do that.
>

As long as I compile on macOS, I guess I'll be bothered enough to try to
keep the warnings at bay.

[-- Attachment #2: Type: text/html, Size: 1345 bytes --]

Re: [PATCH] Clean up a couple of compiler warnings

[Perry E. Metzger]

On Sun, 21 May 2017 20:00:11 +0000 Philipp Stephani
<p.stephani2@gmail.com> wrote:
> For Clang, it'd probably be better to use -Weverything and then
> disable individual warnings with -Wno... instead of checking for
> the existence of every warning flag. That would make configure runs
> faster and the command line shorter.

clang's -Weverything is like an options lamp test. There's an
enormous number of things there, and many aren't even properly
documented. You really don't want to use it -- the sheer number of
things you'll need to disable is astonishing, and with every new
iteration of clang there's an ocean more flags there. I think this
solution should be avoided.

Perry
-- 
Perry E. Metzger		perry@piermont.com