Re: Silence two Clang warnings

Re: Silence two Clang warnings

[Paul Eggert]

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

Part of the attached patch, recently installed into master, would be 
automatically reverted the next time we merged from gnulib, since 
strftime.c is copied from Gnulib. I'm not getting a warning when 
compiling with Clang, so I'm puzzled as to why the patch was needed. 
Here's how I build (on Fedora 25):

./configure --enable-gcc-warnings CC=clang

Fedora 25 is running clang version 3.9.1 (tags/RELEASE_391/final).

Background: I would rather avoid the need for this sort of patch, as we 
shouldn't have to complicate the code just to pacify a Clang false 
alarm. If an older Clang is generating a false alarm, let's just ignore 
the diagnostic. If it's a newer Clang, let's change the options to Clang 
to suppress the diagnostic.


[-- Attachment #2: 0001-Silence-two-Clang-warnings-by-introducing-additional.patch --]
[-- Type: text/x-patch, Size: 2298 bytes --]

From e408e9aa030a00cb1a61acdc729e8d6786b25fe3 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Tue, 13 Jun 2017 13:55:44 +0200
Subject: [PATCH] Silence two Clang warnings by introducing additional local
 variables

* lib/strftime.c (libc_hidden_def):
* lib-src/make-docfile.c (put_filename): Introduce local variables to
silence Clang warnings.
---
 lib-src/make-docfile.c |  6 +++++-
 lib/strftime.c         | 27 ++++++++++++++++-----------
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/lib-src/make-docfile.c b/lib-src/make-docfile.c
index 9470bd6..85bcc8b 100644
--- a/lib-src/make-docfile.c
+++ b/lib-src/make-docfile.c
@@ -224,7 +224,11 @@ put_filename (char *filename)
 
   for (tmp = filename; *tmp; tmp++)
     {
-      if (IS_DIRECTORY_SEP (*tmp))
+      /* Use separate variable to silence a Clang warning on macOS.
+         Clang takes offence of the additional set of parantheses
+         generated by the macro.  */
+      bool is_sep = IS_DIRECTORY_SEP (*tmp);
+      if (is_sep)
 	filename = tmp + 1;
     }
 
diff --git a/lib/strftime.c b/lib/strftime.c
index 99bee4e..18c899d 100644
--- a/lib/strftime.c
+++ b/lib/strftime.c
@@ -1123,18 +1123,23 @@ __strftime_internal (STREAM_OR_CHAR_T *s, STRFTIME_ARG (size_t maxsize)
           if (modifier == L_('E'))
             goto bad_format;
 
-          number_value = ns;
-          if (width == -1)
-            width = 9;
-          else
-            {
-              /* Take an explicit width less than 9 as a precision.  */
-              int j;
-              for (j = width; j < 9; j++)
-                number_value /= 10;
-            }
+          {
+            /* Use a new variable here instead of reusing number_value
+               because Clang complains about the self-assignment
+               generated by DO_NUMBER.  */
+            ptrdiff_t n = ns;
+            if (width == -1)
+              width = 9;
+            else
+              {
+                /* Take an explicit width less than 9 as a precision.  */
+                int j;
+                for (j = width; j < 9; j++)
+                  n /= 10;
+              }
 
-          DO_NUMBER (width, number_value);
+            DO_NUMBER (width, n);
+          }
 #endif
 
         case L_('n'):
-- 
2.9.4

Re: Silence two Clang warnings

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am Di., 13. Juni 2017 um 22:44 Uhr:

> Part of the attached patch, recently installed into master, would be
> automatically reverted the next time we merged from gnulib, since
> strftime.c is copied from Gnulib. I'm not getting a warning when
> compiling with Clang, so I'm puzzled as to why the patch was needed.
> Here's how I build (on Fedora 25):
>
> ./configure --enable-gcc-warnings CC=clang
>
> Fedora 25 is running clang version 3.9.1 (tags/RELEASE_391/final).
>

I get this on macOS with -O3 and -enable-gcc-warnings, using Apple's fork
of Clang (i.e. the default system compiler).


>
> Background: I would rather avoid the need for this sort of patch, as we
> shouldn't have to complicate the code just to pacify a Clang false
> alarm. If an older Clang is generating a false alarm, let's just ignore
> the diagnostic. If it's a newer Clang, let's change the options to Clang
> to suppress the diagnostic.
>
>
I'd prefer to not suppress the diagnostic globally, because these warnings
are generally useful in other places.
Would you accept introducing pragmas to temporarily disable the warnings in
Clang (#pragma clang diagnostic push/ignore/pop), with an explanation why
they are false positives in these cases?

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

Re: Silence two Clang warnings

[Paul Eggert]

On 06/15/2017 06:11 AM, Philipp Stephani wrote:
> Would you accept introducing pragmas to temporarily disable the 
> warnings in Clang (#pragma clang diagnostic push/ignore/pop), with an 
> explanation why they are false positives in these cases? 

That depends on what the warnings are and how useful they are elsewhere. 
These particular warnings do not seem to be useful for Emacs. For 
example, it is common practice in Emacs's C macros to parenthesize 
expressions to avoid operator-precedence confusion, and if Clang is 
complaining about the "extra" parentheses in the preprocessor output 
then that's clearly a warning that we should just shut off. Our goal is 
clarity and reliability, not Clang pacification.

Re: Silence two Clang warnings

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am Do., 15. Juni 2017 um 20:56 Uhr:

> On 06/15/2017 06:11 AM, Philipp Stephani wrote:
> > Would you accept introducing pragmas to temporarily disable the
> > warnings in Clang (#pragma clang diagnostic push/ignore/pop), with an
> > explanation why they are false positives in these cases?
>
> That depends on what the warnings are and how useful they are elsewhere.
> These particular warnings do not seem to be useful for Emacs. For
> example, it is common practice in Emacs's C macros to parenthesize
> expressions to avoid operator-precedence confusion, and if Clang is
> complaining about the "extra" parentheses in the preprocessor output
> then that's clearly a warning that we should just shut off. Our goal is
> clarity and reliability, not Clang pacification.
>
>
Given that that message only appears with CFLAGS=-save-temps, we probably
don't need to disable it. With my usual configuration
(--enable-gcc-warnings, CFLAGS='-O0 -ggdb3') there are no warnings.

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

Re: Silence two Clang warnings

[Paul Eggert]

On 06/15/2017 12:23 PM, Philipp Stephani wrote:
> Given that that message only appears with CFLAGS=-save-temps, we 
> probably don't need to disable it. With my usual configuration 
> (--enable-gcc-warnings, CFLAGS='-O0 -ggdb3') there are no warnings. 

OK, I reverted that change then. This sounds like a bug in how Clang 
generates warnings, as --save-temps shouldn't affect warnings like that.

Re: Silence two Clang warnings

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am Do., 15. Juni 2017 um 21:35 Uhr:

> On 06/15/2017 12:23 PM, Philipp Stephani wrote:
> > Given that that message only appears with CFLAGS=-save-temps, we
> > probably don't need to disable it. With my usual configuration
> > (--enable-gcc-warnings, CFLAGS='-O0 -ggdb3') there are no warnings.
>
> OK, I reverted that change then. This sounds like a bug in how Clang
> generates warnings, as --save-temps shouldn't affect warnings like that.
>
>
Yes, it seems both Clang and GCC have such bugs. See
https://bugs.llvm.org/show_bug.cgi?id=22949 for Clang and
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57201 for GCC. They are easily
reproducible, but apparently nobody treats them as high priority.

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