Discussion:
[PATCH] Avoid one "parameter unused" warnings.
Michal Privoznik
2018-01-01 09:18:49 UTC
Permalink
* lib/stat-time.h (stat_time_normalize):
Mark "st" as used.

Signed-off-by: Michal Privoznik <***@redhat.com>
---
lib/stat-time.h | 2 ++
1 file changed, 2 insertions(+)

diff --git a/lib/stat-time.h b/lib/stat-time.h
index 5f8bf4e12..11acbfb1f 100644
--- a/lib/stat-time.h
+++ b/lib/stat-time.h
@@ -244,6 +244,8 @@ stat_time_normalize (int result, struct stat *st)
}
}
#endif
+ /* Avoid a "parameter unused" warning. */
+ (void) st;
return result;
}
--
2.13.6
Paul Eggert
2018-01-01 09:53:10 UTC
Permalink
Thanks, but -Wunused-parameter issues so many false alarms that I think I'd
rather just ask people to compile with -Wno-unused-parameter. That's what
coreutils does, and Emacs, and so forth.

By the way, this thread duplicates one of a couple of days ago, starting here:

http://lists.gnu.org/archive/html/bug-gnulib/2017-12/msg00077.html

Is there some new project using Gnulib that is causing these new messages? If
so, perhaps I should propose a patch to it to pacify GCC without having to
pollute the code with void no-op casts.
Michal Privoznik
2018-01-01 11:56:21 UTC
Permalink
Post by Paul Eggert
Thanks, but -Wunused-parameter issues so many false alarms that I think
I'd rather just ask people to compile with -Wno-unused-parameter. That's
what coreutils does, and Emacs, and so forth.
Can you elaborate on this please? I'm failing to see why this would be a
false positive. Despite this function being inlined, compiler might
decide to not inline it and thus treat it as any other function in which
case the variable is unused indeed.
Post by Paul Eggert
http://lists.gnu.org/archive/html/bug-gnulib/2017-12/msg00077.html
Ah, haven't noticed, sorry.
Post by Paul Eggert
Is there some new project using Gnulib that is causing these new
messages? If so, perhaps I should propose a patch to it to pacify GCC
without having to pollute the code with void no-op casts.
Yes, libvirt. However, libvirt's no newbie to gnulib. We've used it for
ages.

Michal
Paul Eggert
2018-01-01 18:41:50 UTC
Permalink
Post by Michal Privoznik
I'm failing to see why this would be a
false positive.
It's a false positive in the sense that there is no bug here. That is, the
Gnulib patch would mean that we would be spending maintenance effort to
complicate code merely to pacify a compiler, not to fix a real problem.
Sometimes this is worth doing, if the compiler's warning is typically a valid
one and is therefore useful to catch bugs. However, this particular case does
not seem to be worth doing, as this particular compiler warning is too often a
false alarm and the bugs it catches are not worth the maintenance effort.
Post by Michal Privoznik
Despite this function being inlined, compiler might
decide to not inline it and thus treat it as any other function in which
case the variable is unused indeed.
I don't see why inlining is relevant. Regardless of whether the compiler decides
to inline at the machine-code level, the extra source-code clutter does not fix
any real bug.

I attempted to reproduce the problem by running these commands on Fedora 27 x86-64:

git clone git://libvirt.org/libvirt.git
cd libvirt
./autogen.sh
make -j5

This worked fine for me, so I can't easily test a fix; however, perhaps the
attached patch to libvirt will fix things for you.
Michal Privoznik
2018-01-02 06:21:37 UTC
Permalink
Post by Paul Eggert
Post by Michal Privoznik
I'm failing to see why this would be a
false positive.
It's a false positive in the sense that there is no bug here. That is,
the Gnulib patch would mean that we would be spending maintenance effort
to complicate code merely to pacify a compiler, not to fix a real
problem. Sometimes this is worth doing, if the compiler's warning is
typically a valid one and is therefore useful to catch bugs. However,
this particular case does not seem to be worth doing, as this particular
compiler warning is too often a false alarm and the bugs it catches are
not worth the maintenance effort.
I beg to disagree. For reference I'm pasting skeleton of the function:

inline in
stat_time_normalize(int result, struct stat *st)
{
#if $cond

#endif
return result;
}

If $cond is false (i.e. I'm not building on SUN), the @st variable is
unused indeed. True, it's not a serious error nor bug. However, it's not
a false positive either - we told compiler to warn us in this case and
it did. If you guys don't want to 'fix' it, that's okay. I was more
focused on the false positivity. BTW: there are already instances of my
patch (true, from ancient history), e.g. 81eb84868d.
Post by Paul Eggert
Post by Michal Privoznik
Despite this function being inlined, compiler might
decide to not inline it and thus treat it as any other function in which
case the variable is unused indeed.
I don't see why inlining is relevant. Regardless of whether the compiler
decides to inline at the machine-code level, the extra source-code
clutter does not fix any real bug.
git clone git://libvirt.org/libvirt.git
cd libvirt
./autogen.sh
make -j5
This worked fine for me, so I can't easily test a fix; however, perhaps
the attached patch to libvirt will fix things for you.
Try running 'make syntax-check' which fails because of gnulib's
copyright_check. And when updating the submodule I've found this bug.

BTW: the patch of yours don't help really. Firstly, unused-parameter
sneaks back in (through -Wextra I assume). And secondly, in libvirt we
want to use Wunused-parameter and we do mark unused parameters
explicitly (see ATTRIBUTE_UNUSED macro).

Michal
Paul Eggert
2018-01-02 07:38:40 UTC
Permalink
it's not a false positive either
It depends on what we're looking for. I'm looking for bugs. To take an extreme
example, if GCC warned about every character string literal containing an
unescaped ASCII apostrophe, on the grounds that the apostrophe could be a typo
and program quality would be improved by requiring programmers to manually check
and explicitly escape every string-literal apostrophe, I'd disable such a
warning first thing. Any benefits of the warning would not be worth the hassle.

The warning we're talking about is not as bad as this hypothetical example. But
it's pretty bad. I'd rather not contort good code to pacify it.
Try running 'make syntax-check' which fails because of gnulib's
copyright_check.
This sounds like a different and unrelated problem.
BTW: the patch of yours don't help really. Firstly, unused-parameter
sneaks back in (through -Wextra I assume). And secondly, in libvirt we
want to use Wunused-parameter and we do mark unused parameters
explicitly (see ATTRIBUTE_UNUSED macro).
In that case, I suggest doing what coreutils and other programs do: use a
different set of warnings for Gnulib code than for the libvirt code. You can
specify -Wno-unused-parameter just for Gnulib; this will override -Wextra.
Michal Privoznik
2018-01-02 08:20:37 UTC
Permalink
Post by Paul Eggert
it's not a false positive either
It depends on what we're looking for. I'm looking for bugs. To take an
extreme example, if GCC warned about every character string literal
containing an unescaped ASCII apostrophe, on the grounds that the
apostrophe could be a typo and program quality would be improved by
requiring programmers to manually check and explicitly escape every
string-literal apostrophe, I'd disable such a warning first thing. Any
benefits of the warning would not be worth the hassle.
The warning we're talking about is not as bad as this hypothetical
example. But it's pretty bad. I'd rather not contort good code to pacify
it.
Try running 'make syntax-check' which fails because of gnulib's
copyright_check.
This sounds like a different and unrelated problem.
Yes it is. It's just that when I wanted to update gnulib submodule I ran
into this problem of unused parameter.
Post by Paul Eggert
BTW: the patch of yours don't help really. Firstly, unused-parameter
sneaks back in (through -Wextra I assume). And secondly, in libvirt we
want to use Wunused-parameter and we do mark unused parameters
explicitly (see ATTRIBUTE_UNUSED macro).
In that case, I suggest doing what coreutils and other programs do: use
a different set of warnings for Gnulib code than for the libvirt code.
You can specify -Wno-unused-parameter just for Gnulib; this will
override -Wextra.
I don't think this is going to fly. Problem is a .c file includes
stat-time.h from gnulib:

make[3]: Entering directory '/home/zippy/work/libvirt/libvirt.git/src'
CC storage/libvirt_driver_storage_impl_la-storage_util.lo
In file included from storage/storage_util.c:67:0:
../gnulib/lib/stat-time.h: In function 'stat_time_normalize':
../gnulib/lib/stat-time.h:215:47: error: unused parameter 'st'
[-Werror=unused-parameter]
stat_time_normalize (int result, struct stat *st)
^~
cc1: all warnings being treated as errors


Different CFLAGS for gnulib won't work IMO. Maybe we can use #pragma to
disable warnings when #including the gnulib file? Although, that's very
unfortunate.

Michal
Eric Blake
2018-01-02 21:48:08 UTC
Permalink
Post by Paul Eggert
Thanks, but -Wunused-parameter issues so many false alarms that I think
I'd rather just ask people to compile with -Wno-unused-parameter. That's
what coreutils does, and Emacs, and so forth.
http://lists.gnu.org/archive/html/bug-gnulib/2017-12/msg00077.html
Is there some new project using Gnulib that is causing these new
messages? If so, perhaps I should propose a patch to it to pacify GCC
without having to pollute the code with void no-op casts.
Note that this patch is copying the style that is already present in
get_stat_birthtime_ns(), so on that grounds, I'd argue that we WANT the
patch for consistency. But why not use _GL_UNUSED instead of
cast-to-void (after all, gcc does NOT warn if you mark something with
_GL_UNUSED but then actually use it; precisely because of situations
where the use is conditional based on #if)? As in:

diff --git i/lib/stat-time.h w/lib/stat-time.h
index 5f8bf4e12..104f53766 100644
--- i/lib/stat-time.h
+++ w/lib/stat-time.h
@@ -102,15 +102,13 @@ get_stat_mtime_ns (struct stat const *st)

/* Return the nanosecond component of *ST's birth time. */
_GL_STAT_TIME_INLINE long int _GL_ATTRIBUTE_PURE
-get_stat_birthtime_ns (struct stat const *st)
+get_stat_birthtime_ns (struct stat const *st _GL_UNUSED)
{
# if defined HAVE_STRUCT_STAT_ST_BIRTHTIMESPEC_TV_NSEC
return STAT_TIMESPEC (st, st_birthtim).tv_nsec;
# elif defined HAVE_STRUCT_STAT_ST_BIRTHTIMENSEC
return STAT_TIMESPEC_NS (st, st_birthtim);
# else
- /* Avoid a "parameter unused" warning. */
- (void) st;
return 0;
# endif
}
@@ -160,7 +158,7 @@ get_stat_mtime (struct stat const *st)
/* Return *ST's birth time, if available; otherwise return a value
with tv_sec and tv_nsec both equal to -1. */
_GL_STAT_TIME_INLINE struct timespec _GL_ATTRIBUTE_PURE
-get_stat_birthtime (struct stat const *st)
+get_stat_birthtime (struct stat const *st _GL_UNUSED)
{
struct timespec t;

@@ -184,8 +182,6 @@ get_stat_birthtime (struct stat const *st)
/* Birth time is not supported. */
t.tv_sec = -1;
t.tv_nsec = -1;
- /* Avoid a "parameter unused" warning. */
- (void) st;
#endif

#if (defined HAVE_STRUCT_STAT_ST_BIRTHTIMESPEC_TV_NSEC \
@@ -212,7 +208,7 @@ get_stat_birthtime (struct stat const *st)
errno to EOVERFLOW if normalization overflowed. This function
is intended to be private to this .h file. */
_GL_STAT_TIME_INLINE int
-stat_time_normalize (int result, struct stat *st)
+stat_time_normalize (int result, struct stat *st _GL_UNUSED)
{
#if defined __sun && defined STAT_TIMESPEC
if (result == 0)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Eric Blake
2018-01-02 21:56:55 UTC
Permalink
Commit 2c5d5587 causes warnings on non-Sun systems when compiled
under -Wunused-parameter; we've previously tweaked code in commit
81eb8486 to avoid such warnings. Prefer an attribute rather than
a cast to void (the attribute is always okay to apply; gcc
interprets it as 'may be unused', not 'must not be used', precisely
to cater to #if chains where determining whether or not the
parameter is used gets hairy).

* lib/stat-time.h (get_stat_birthtime_ns, get_stat_birthtime):
Prefer attribute over cast-to-void.
(stat_time_normalize): Mark st as potentially unused.

Signed-off-by: Eric Blake <***@redhat.com>
---
ChangeLog | 6 ++++++
lib/stat-time.h | 10 +++-------
2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 24a63ef2c..dd144ed31 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2018-01-02 Eric Blake <***@redhat.com>
+
+ * lib/stat-time.h (get_stat_birthtime_ns, get_stat_birthtime):
+ Prefer attribute over cast-to-void.
+ (stat_time_normalize): Mark st as potentially unused.
+
2018-01-01 Jim Meyering <***@fb.com>

update-copyright: add code to handle more special cases
diff --git a/lib/stat-time.h b/lib/stat-time.h
index 5f8bf4e12..104f53766 100644
--- a/lib/stat-time.h
+++ b/lib/stat-time.h
@@ -102,15 +102,13 @@ get_stat_mtime_ns (struct stat const *st)

/* Return the nanosecond component of *ST's birth time. */
_GL_STAT_TIME_INLINE long int _GL_ATTRIBUTE_PURE
-get_stat_birthtime_ns (struct stat const *st)
+get_stat_birthtime_ns (struct stat const *st _GL_UNUSED)
{
# if defined HAVE_STRUCT_STAT_ST_BIRTHTIMESPEC_TV_NSEC
return STAT_TIMESPEC (st, st_birthtim).tv_nsec;
# elif defined HAVE_STRUCT_STAT_ST_BIRTHTIMENSEC
return STAT_TIMESPEC_NS (st, st_birthtim);
# else
- /* Avoid a "parameter unused" warning. */
- (void) st;
return 0;
# endif
}
@@ -160,7 +158,7 @@ get_stat_mtime (struct stat const *st)
/* Return *ST's birth time, if available; otherwise return a value
with tv_sec and tv_nsec both equal to -1. */
_GL_STAT_TIME_INLINE struct timespec _GL_ATTRIBUTE_PURE
-get_stat_birthtime (struct stat const *st)
+get_stat_birthtime (struct stat const *st _GL_UNUSED)
{
struct timespec t;

@@ -184,8 +182,6 @@ get_stat_birthtime (struct stat const *st)
/* Birth time is not supported. */
t.tv_sec = -1;
t.tv_nsec = -1;
- /* Avoid a "parameter unused" warning. */
- (void) st;
#endif

#if (defined HAVE_STRUCT_STAT_ST_BIRTHTIMESPEC_TV_NSEC \
@@ -212,7 +208,7 @@ get_stat_birthtime (struct stat const *st)
errno to EOVERFLOW if normalization overflowed. This function
is intended to be private to this .h file. */
_GL_STAT_TIME_INLINE int
-stat_time_normalize (int result, struct stat *st)
+stat_time_normalize (int result, struct stat *st _GL_UNUSED)
{
#if defined __sun && defined STAT_TIMESPEC
if (result == 0)
--
2.14.3
Jim Meyering
2018-01-03 01:27:39 UTC
Permalink
Post by Eric Blake
Commit 2c5d5587 causes warnings on non-Sun systems when compiled
under -Wunused-parameter; we've previously tweaked code in commit
81eb8486 to avoid such warnings. Prefer an attribute rather than
a cast to void (the attribute is always okay to apply; gcc
interprets it as 'may be unused', not 'must not be used', precisely
to cater to #if chains where determining whether or not the
parameter is used gets hairy).
Prefer attribute over cast-to-void.
(stat_time_normalize): Mark st as potentially unused.
Thanks. That sounds like the right approach. We have to hold
user-included headers to a higher standard.
Paul Eggert
2018-01-03 01:32:10 UTC
Permalink
Post by Jim Meyering
Thanks. That sounds like the right approach. We have to hold
user-included headers to a higher standard.
Right, I missed that. Thanks, Eric.
Tim Rühsen
2018-01-03 10:53:40 UTC
Permalink
... here is another one in lib/cdef.h occurring with a MinGW build.


In file included from ../lib/libc-config.h:149:0,
from ../lib/glob.h:24,
from utils.c:35:
../lib/cdefs.h:298:6: warning: "__USE_FORTIFY_LEVEL" is not defined,
evaluates to 0 [-Wundef]
# if __USE_FORTIFY_LEVEL > 0
^~~~~~~~~~~~~~~~~~~


With Best Regards, Tim
Post by Jim Meyering
Post by Eric Blake
Commit 2c5d5587 causes warnings on non-Sun systems when compiled
under -Wunused-parameter; we've previously tweaked code in commit
81eb8486 to avoid such warnings. Prefer an attribute rather than
a cast to void (the attribute is always okay to apply; gcc
interprets it as 'may be unused', not 'must not be used', precisely
to cater to #if chains where determining whether or not the
parameter is used gets hairy).
Prefer attribute over cast-to-void.
(stat_time_normalize): Mark st as potentially unused.
Thanks. That sounds like the right approach. We have to hold
user-included headers to a higher standard.
Eric Blake
2018-01-03 15:07:54 UTC
Permalink
Post by Tim Rühsen
... here is another one in lib/cdef.h occurring with a MinGW build.
In file included from ../lib/libc-config.h:149:0,
from ../lib/glob.h:24,
../lib/cdefs.h:298:6: warning: "__USE_FORTIFY_LEVEL" is not defined,
evaluates to 0 [-Wundef]
# if __USE_FORTIFY_LEVEL > 0
^~~~~~~~~~~~~~~~~~~
How come it doesn't trigger a warning in lib/stdio.in.h? At any rate,
-Wundef is slightly different than -Wunused-parameter, but I _do_ see
cases where we have catered to it in .h files:

lib/cdefs.h:#if defined __GNUC__ && __GNUC__ >= 2
lib/file-set.h:#if defined __GNUC__ && ((__GNUC__ == 3 && __GNUC_MINOR__
Post by Tim Rühsen
= 3) || __GNUC__ > 3)
lib/gettext.h:#if defined ENABLE_NLS && ENABLE_NLS
lib/obstack.h:# if !defined __GNUC_MINOR__ || __GNUC__ * 1000 +
__GNUC_MINOR__ < 2008

and since cdefs.h is in that group, feel free to submit a patch to have
the use of __USE_FORTIFY_LEVEL pacify -Wundef.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Tim Rühsen
2018-01-03 15:19:10 UTC
Permalink
Post by Eric Blake
Post by Tim Rühsen
... here is another one in lib/cdef.h occurring with a MinGW build.
In file included from ../lib/libc-config.h:149:0,
from ../lib/glob.h:24,
../lib/cdefs.h:298:6: warning: "__USE_FORTIFY_LEVEL" is not defined,
evaluates to 0 [-Wundef]
# if __USE_FORTIFY_LEVEL > 0
^~~~~~~~~~~~~~~~~~~
How come it doesn't trigger a warning in lib/stdio.in.h? At any rate,
-Wundef is slightly different than -Wunused-parameter, but I _do_ see
lib/cdefs.h:#if defined __GNUC__ && __GNUC__ >= 2
lib/file-set.h:#if defined __GNUC__ && ((__GNUC__ == 3 && __GNUC_MINOR__
Post by Tim Rühsen
= 3) || __GNUC__ > 3)
lib/gettext.h:#if defined ENABLE_NLS && ENABLE_NLS
lib/obstack.h:# if !defined __GNUC_MINOR__ || __GNUC__ * 1000 +
__GNUC_MINOR__ < 2008
and since cdefs.h is in that group, feel free to submit a patch to have
the use of __USE_FORTIFY_LEVEL pacify -Wundef.
There was a patch attached to email...

Since lib/stdio.in.h doesn't trigger here I am unsure if I should patch
it as well.

Regards, Tim
Eric Blake
2018-01-03 21:29:33 UTC
Permalink
Post by Tim Rühsen
Post by Eric Blake
and since cdefs.h is in that group, feel free to submit a patch to have
the use of __USE_FORTIFY_LEVEL pacify -Wundef.
There was a patch attached to email...
Ah, I had overlooked it because you top-posted, and I didn't scroll past
the quoted content to see your patch.
Post by Tim Rühsen
Since lib/stdio.in.h doesn't trigger here I am unsure if I should patch
it as well.
It's easier to wait until someone reports actual issues.
Post by Tim Rühsen
From 8706dfd2673acfd668a250a6942cc437f5636154 Mon Sep 17 00:00:00 2001
Date: Wed, 3 Jan 2018 11:50:07 +0100
Subject: [PATCH] Fix -Wundef in user-included header lib/cdef.h
We still use ChangeLog-style comments in the commit message (and/or
ChangeLog itself) for gnulib commits; it's also nice to call out which
previous commit to cdefs.h already added protection for -Wundef
compilation. But with those additions, I'm okay with your patch; I'll
wait a day or so to see if anyone else chimes in.
Post by Tim Rühsen
---
lib/cdefs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/cdefs.h b/lib/cdefs.h
index ec1fd4106..db72af433 100644
--- a/lib/cdefs.h
+++ b/lib/cdefs.h
@@ -295,7 +295,7 @@
#if __GNUC_PREREQ (3,4)
# define __attribute_warn_unused_result__ \
__attribute__ ((__warn_unused_result__))
-# if __USE_FORTIFY_LEVEL > 0
+# if defined __USE_FORTIFY_LEVEL && __USE_FORTIFY_LEVEL > 0
# define __wur __attribute_warn_unused_result__
# endif
#else
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Paul Eggert
2018-01-03 22:09:29 UTC
Permalink
But with those additions, I'm okay with your patch; I'll > wait a day or so to see if anyone else chimes in.
Looks good to me, too. Thanks.
Tim Rühsen
2018-01-04 09:45:12 UTC
Permalink
Post by Eric Blake
We still use ChangeLog-style comments in the commit message (and/or
ChangeLog itself) for gnulib commits; it's also nice to call out which
previous commit to cdefs.h already added protection for -Wundef
compilation. But with those additions, I'm okay with your patch; I'll
wait a day or so to see if anyone else chimes in.
Added GNU style commit message, unsure what you mean with "which
previous commit to cdefs.h already added protection for -Wundef".
Can't find any (from commit messages).

Regards, Tim
Bruno Haible
2018-01-04 10:02:09 UTC
Permalink
Post by Tim Rühsen
Added GNU style commit message
Thanks for the patch. Applied.

Bruno
Eric Blake
2018-01-04 15:21:11 UTC
Permalink
Post by Tim Rühsen
Post by Eric Blake
We still use ChangeLog-style comments in the commit message (and/or
ChangeLog itself) for gnulib commits; it's also nice to call out which
previous commit to cdefs.h already added protection for -Wundef
compilation. But with those additions, I'm okay with your patch; I'll
wait a day or so to see if anyone else chimes in.
Added GNU style commit message, unsure what you mean with "which
previous commit to cdefs.h already added protection for -Wundef".
Can't find any (from commit messages).
Here's what I did in the recent commit 3206134a:

Commit 2c5d5587 causes warnings on non-Sun systems when compiled
under -Wunused-parameter; we've previously tweaked code in commit
81eb8486 to avoid such warnings.

I found those commit ids by using git gui blame. For your patch, I note
that cdefs.h already has -Wundef protection on __GNUC__ (this is the
command I used to generate examples mentioned earlier in the thread:
git grep '# *if.*defined \([A-Z0-9_]*\) .*\1' lib/*.h
); so I did a blame to see where that protection was first added; but it
appears that

#if defined __GNUC__ && __GNUC__ >= 2

was present in that file since its introduction in commit 38885c86, and
not something added after the fact in a later gnulib commit.

At any rate, since your commit is already applied to gnulib.git, we
don't have to worry about enhancing the commit message to include any
further git archaeology.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Tim Rühsen
2018-01-04 15:34:45 UTC
Permalink
Post by Eric Blake
Post by Tim Rühsen
Post by Eric Blake
We still use ChangeLog-style comments in the commit message (and/or
ChangeLog itself) for gnulib commits; it's also nice to call out which
previous commit to cdefs.h already added protection for -Wundef
compilation. But with those additions, I'm okay with your patch; I'll
wait a day or so to see if anyone else chimes in.
Added GNU style commit message, unsure what you mean with "which
previous commit to cdefs.h already added protection for -Wundef".
Can't find any (from commit messages).
Commit 2c5d5587 causes warnings on non-Sun systems when compiled
under -Wunused-parameter; we've previously tweaked code in commit
81eb8486 to avoid such warnings.
I found those commit ids by using git gui blame. For your patch, I note
that cdefs.h already has -Wundef protection on __GNUC__ (this is the
git grep '# *if.*defined \([A-Z0-9_]*\) .*\1' lib/*.h
); so I did a blame to see where that protection was first added; but it
appears that
#if defined __GNUC__ && __GNUC__ >= 2
was present in that file since its introduction in commit 38885c86, and
not something added after the fact in a later gnulib commit.
Thanks for sharing.

My (maybe over-)simplified approach was

git log lib/cdefs.h

and to read the commit messages - and they didn't mention an explicit
-Wundef protection.

Regards, Tim

Bruno Haible
2018-01-01 09:55:46 UTC
Permalink
Hi,
Post by Michal Privoznik
diff --git a/lib/stat-time.h b/lib/stat-time.h
index 5f8bf4e12..11acbfb1f 100644
--- a/lib/stat-time.h
+++ b/lib/stat-time.h
@@ -244,6 +244,8 @@ stat_time_normalize (int result, struct stat *st)
}
}
#endif
+ /* Avoid a "parameter unused" warning. */
+ (void) st;
return result;
}
Thanks, but this was already discussed and rejected recently:
https://lists.gnu.org/archive/html/bug-gnulib/2017-12/msg00078.html

Bruno
Eric Blake
2018-01-02 21:41:35 UTC
Permalink
Post by Bruno Haible
Hi,
Post by Michal Privoznik
diff --git a/lib/stat-time.h b/lib/stat-time.h
index 5f8bf4e12..11acbfb1f 100644
--- a/lib/stat-time.h
+++ b/lib/stat-time.h
@@ -244,6 +244,8 @@ stat_time_normalize (int result, struct stat *st)
}
}
#endif
+ /* Avoid a "parameter unused" warning. */
+ (void) st;
return result;
}
https://lists.gnu.org/archive/html/bug-gnulib/2017-12/msg00078.html
Compiling gnulib .c files with -Wno-unused-parameter makes total sense
to me (we already recommend that projects use a smaller set of -W for
gnulib .c files than for their own files, if they like a particularly
strict warning). But for inline functions in .h files, where we have
less control of which -W options are in effect, it makes more sense to
me to accept some form of patch to shut up the compiler. I'm not sure
that a cast-to-void is the best approach, or if we can add some compiler
attribute instead of a pragma to push/pop a temporary warning
relaxation, but I'm not sure we should discard this patch wholesale
merely because not all projects use -Wunused-parameter.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Loading...