You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jonathan Gilbert <o2...@sneakemail.com> on 2005/10/25 05:26:28 UTC
[PATCH] Move unused parameter warning suppression into a macro
At David James recommendation, I have placed the macro into svn_types.h.
This seemed the least illogical place to put it, but none of the existing
headers were really a perfect match. :-)
Log:
Move unused parameter warning suppression into a macro instead of directly
using "(void)p;" or "p=p;" in otherwise semantically-oriented source code.
Index: subversion/include/svn_types.h
===================================================================
--- subversion/include/svn_types.h (revision 16987)
+++ subversion/include/svn_types.h (working copy)
@@ -84,6 +84,16 @@
/** @} */
+/** A macro to suppress some compilers' "Parameter is not used" warnings */
+#ifndef SVN_UNUSED_PARAMETER
+#define SVN_UNUSED_PARAMETER(x) ((void)x)
+/* Possible alternative:
+ *
+ * #define SVN_UNUSED_PARAMETER(x) ((x) = (x))
+ */
+#endif /* SVN_UNUSED_PARAMETER */
+
+
/** The various types of nodes in the Subversion filesystem. */
typedef enum
{
Index: subversion/libsvn_wc/adm_files.c
===================================================================
--- subversion/libsvn_wc/adm_files.c (revision 16987)
+++ subversion/libsvn_wc/adm_files.c (working copy)
@@ -57,7 +57,7 @@
svn_boolean_t
svn_wc_is_adm_dir (const char *name, apr_pool_t *pool)
{
- (void)pool; /* Silence compiler warnings about unused parameter. */
+ SVN_UNUSED_PARAMETER(pool);
return (0 == strcmp (name, adm_dir_name)
|| 0 == strcmp (name, default_adm_dir_name));
}
@@ -66,7 +66,7 @@
const char *
svn_wc_get_adm_dir (apr_pool_t *pool)
{
- (void)pool; /* Silence compiler warnings about unused parameter. */
+ SVN_UNUSED_PARAMETER(pool);
return adm_dir_name;
}
Index: subversion/libsvn_subr/config.c
===================================================================
--- subversion/libsvn_subr/config.c (revision 16987)
+++ subversion/libsvn_subr/config.c (working copy)
@@ -326,8 +326,8 @@
option->expanded = FALSE;
}
- (void)(baton); /* Unused parameter. */
- (void)(section); /* Unused parameter. */
+ SVN_UNUSED_PARAMETER(baton);
+ SVN_UNUSED_PARAMETER(section);
return FALSE;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Move unused parameter warning suppression into a macro
Posted by David James <ja...@gmail.com>.
On 10/27/05, Julian Foad <ju...@btopenworld.com> wrote:
> 1) Mark the parameter where it appears in the parameter list. This is the best
> way for compilers that support it. We know GCC supports it; can anyone tell us
> about others?
This solution seems too complex for a small problem.
> 2) Mark the parameter in the function body. Compatible with all compilers, but
> tends to just replace one warning with another: "statement with no effect" for
> the version that goes after declarations; "unused local variable" for the
> version that goes before, except on compilers where (1) is possible.
>
>
> 3) Disable "unused parameter" warnings. (GCC: disabled by default;
> "-Wno-unused-parameter" if required. MSVC: maybe "#pragma
> warning(disable:NNNN)" ?.) This is what we (at least I) do now. The
> disadvantage is that we can't get the benefit of such warnings in functions
> where we did intend to use the parameters. However, in practice I don't think
> we actually gain very much, as we use other methods to ensure our functions
> work correctly (review and testing).
>
>
> Having seen your latest analysis above and written this response, it now looks
> to me like (1) and (3) are both better than (2).
+1 to disable unused parameter warnings, and remove all of the "(void)
parameter" hacks. The "unused parameter" warning is not a particularly
valuable one, so I don't think it's worth overthinking this.
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
Re: [PATCH] Move unused parameter warning suppression into a macro
Posted by Julian Foad <ju...@btopenworld.com>.
Jonathan Gilbert wrote:
>
> I've figured out how to put it into svn_private_config.h.
Good. Note that there are two places: "configure.in" for autoconf systems and
build/generator/ for MS Windows/MSVC.
> I have a proposed change to the definition of the macro. Basically, what's
> happening as I add it into all the places that need it is stuff like this:
>
> svn_prop_kind_t kind = svn_property_kind (NULL, name);
>
> SVN_UNUSED_PARAMETER (pool);
>
> if (kind != svn_prop_regular_kind)
> return svn_error_createf (...);
Yes, that's rather ugly.
> It has to go *after* the local variable declarations because it is a
> statement, which often has the effect of splitting up code. What I was
> thinking would be to allow:
>
> SVN_UNUSED_PARAMETER (pool);
>
> svn_prop_kind_t kind = svn_property_kind (NULL, name);
> if (kind != svn_prop_regular_kind)
> return svn_error_createf
>
> ..by turning the macro into something like:
>
> #define SVN_UNUSED_PARAMETER(x) void *svn_unused_parameter__##x = &x
Yes, that's much nicer, except...
> The compiler would then see a declaration. Now, the compiler might also be
> emitting warnings for unused local variables. However, at least when GCC is
> being used, there is an attribute which can be applied to it which will
> stop it from producing that warning: __attribute__((__unused__)). In
... that's a problem. On other compilers, we're now replacing an unused
parameter with an unused local variable, which doesn't seem like any
improvement to me. On GCC, we could have solved the problem a better way, by
marking the parameter itself "unused".
> addition, if the __deprecated__ attribute is applied to the parameter in
> the function's signature, then the compiler will emit a warning whenever
> code actually does use the parameter. (Deprecation isn't a perfect match to
> the meaning we want, but it's the only attribute I saw which would result
> in a warning.) In order to do this, another macro would also be needed,
> perhaps:
>
> #define SVN_UNUSED_PARAMETER_NAME(x) x __attribute__((__deprecated__))
No, that's silly. Mark the parameter "unused" instead, and make the other
macro evaluate to nothing.
> it occurs to me that the __unused__ attribute could probably also be
> applied directly here, but of course only GCC will support this, so
> SVN_UNUSED_PARAMETER would still be needed.
>
> Another thing that SVN_UNUSED_PARAMETER_NAME could do is simply leave the
> name off for C++ code:
>
> #ifdef __cplusplus
> # define SVN_UNUSED_PARAMETER_NAME(x)
> #elif defined __GCC__
> # define SVN_UNUSED_PARAMETER_NAME(x) x __attribute__((__deprecated__))
> #else
> # define SVN_UNUSED_PARAMETER_NAME(x) x
> #endif
Yes, certainly, if our code is compilable under C++.
> It would mean even more expansion in the size of the code, but it would
I don't know what you mean. What's "it", and what size?
> also mean that we would be able to detect instances where parameters that
> were marked as unused were in fact being used. I'm not sure how important
> this is.
Not very. The whole thing isn't very important. If there's a neat solution,
then it's worth doing something, but it's not worth implementing a complex or
ugly solution.
> Usage would look like this:
>
> static svn_error_t *
> validate_prop (const char *name,
> apr_pool_t * SVN_UNUSED_PARAMETER_NAME (pool))
> {
> SVN_UNUSED_PARAMETER (pool);
And that - having to mark the parameter twice - is ugly. I don't want to see that.
I think we have a choice among three options:
1) Mark the parameter where it appears in the parameter list. This is the best
way for compilers that support it. We know GCC supports it; can anyone tell us
about others?
2) Mark the parameter in the function body. Compatible with all compilers, but
tends to just replace one warning with another: "statement with no effect" for
the version that goes after declarations; "unused local variable" for the
version that goes before, except on compilers where (1) is possible.
3) Disable "unused parameter" warnings. (GCC: disabled by default;
"-Wno-unused-parameter" if required. MSVC: maybe "#pragma
warning(disable:NNNN)" ?.) This is what we (at least I) do now. The
disadvantage is that we can't get the benefit of such warnings in functions
where we did intend to use the parameters. However, in practice I don't think
we actually gain very much, as we use other methods to ensure our functions
work correctly (review and testing).
Having seen your latest analysis above and written this response, it now looks
to me like (1) and (3) are both better than (2).
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Move unused parameter warning suppression into a
macro
Posted by Jonathan Gilbert <o2...@sneakemail.com>.
At 11:51 AM 26/10/2005 +0100, Julian Foad wrote:
[snip]
>In this case, it's not worth splitting the patch into "implement the
macro" and
>"use the macro" because both parts are so simple (even though the latter
part
>is large), so just put it all in a single patch.
Okay.
[snip]
>I'm still thinking about whether we can do better than putting the
definition
>in svn_types.h. We really should try hard to avoid using that file as a
>dumping ground. I'm thinking svn_private_config.h would be better, since
this
>is an implementation detail that users of our libraries don't need to know
>about, and since the appropriate definition might depend on the compilation
>environment. For example, some compilers might complain more about
"(void)x"
>being a statement with no effect than they do about unused parameters.
I've figured out how to put it into svn_private_config.h.
I have a proposed change to the definition of the macro. Basically, what's
happening as I add it into all the places that need it is stuff like this:
svn_prop_kind_t kind = svn_property_kind (NULL, name);
SVN_UNUSED_PARAMETER (pool);
if (kind != svn_prop_regular_kind)
return svn_error_createf (...);
It has to go *after* the local variable declarations because it is a
statement, which often has the effect of splitting up code. What I was
thinking would be to allow:
SVN_UNUSED_PARAMETER (pool);
svn_prop_kind_t kind = svn_property_kind (NULL, name);
if (kind != svn_prop_regular_kind)
return svn_error_createf
..by turning the macro into something like:
#define SVN_UNUSED_PARAMETER(x) void *svn_unused_parameter__##x = &x
The compiler would then see a declaration. Now, the compiler might also be
emitting warnings for unused local variables. However, at least when GCC is
being used, there is an attribute which can be applied to it which will
stop it from producing that warning: __attribute__((__unused__)). In
addition, if the __deprecated__ attribute is applied to the parameter in
the function's signature, then the compiler will emit a warning whenever
code actually does use the parameter. (Deprecation isn't a perfect match to
the meaning we want, but it's the only attribute I saw which would result
in a warning.) In order to do this, another macro would also be needed,
perhaps:
#define SVN_UNUSED_PARAMETER_NAME(x) x __attribute__((__deprecated__))
it occurs to me that the __unused__ attribute could probably also be
applied directly here, but of course only GCC will support this, so
SVN_UNUSED_PARAMETER would still be needed.
Another thing that SVN_UNUSED_PARAMETER_NAME could do is simply leave the
name off for C++ code:
#ifdef __cplusplus
# define SVN_UNUSED_PARAMETER_NAME(x)
#elif defined __GCC__
# define SVN_UNUSED_PARAMETER_NAME(x) x __attribute__((__deprecated__))
#else
# define SVN_UNUSED_PARAMETER_NAME(x) x
#endif
It would mean even more expansion in the size of the code, but it would
also mean that we would be able to detect instances where parameters that
were marked as unused were in fact being used. I'm not sure how important
this is.
Usage would look like this:
static svn_error_t *
validate_prop (const char *name,
apr_pool_t * SVN_UNUSED_PARAMETER_NAME (pool))
{
SVN_UNUSED_PARAMETER (pool);
svn_prop_kind_t kind = svn_property_kind (NULL, name);
if (kind != svn_prop_regular_kind)
return svn_error_createf (...);
return SVN_NO_ERROR;
}
Any comments?
Jonathan Gilbert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Move unused parameter warning suppression into a macro
Posted by Daniel Rall <dl...@finemaltcoding.com>.
On Wed, 26 Oct 2005, Erik Huelsmann wrote:
> > I'm still thinking about whether we can do better than putting the definition
> > in svn_types.h. We really should try hard to avoid using that file as a
> > dumping ground. I'm thinking svn_private_config.h would be better, since this
>
> +1
>
> > is an implementation detail that users of our libraries don't need to know
> > about, and since the appropriate definition might depend on the compilation
> > environment. For example, some compilers might complain more about "(void)x"
> > being a statement with no effect than they do about unused parameters.
>
> Which may mean that at some time we need to implement configure magic
> to check for it... (which means it'll be in svn_private_config.h by
> then)
+1
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Move unused parameter warning suppression into a macro
Posted by Erik Huelsmann <eh...@gmail.com>.
> I'm still thinking about whether we can do better than putting the definition
> in svn_types.h. We really should try hard to avoid using that file as a
> dumping ground. I'm thinking svn_private_config.h would be better, since this
+1
> is an implementation detail that users of our libraries don't need to know
> about, and since the appropriate definition might depend on the compilation
> environment. For example, some compilers might complain more about "(void)x"
> being a statement with no effect than they do about unused parameters.
Which may mean that at some time we need to implement configure magic
to check for it... (which means it'll be in svn_private_config.h by
then)
bye,
Erik.
Re: [PATCH] Move unused parameter warning suppression into a macro
Posted by Julian Foad <ju...@btopenworld.com>.
Jonathan Gilbert wrote:
> At 12:21 PM 25/10/2005 +0100, Julian Foad wrote:
>
>>If you mean all the "unused parameter" warnings, then yes; I hope Jonathan will
>>do that.
>
> If you commit the basic patch, then I'll do that. Otherwise, I'll be stuck
> with trying to split it into two diffs against HEAD, which I don't feel
> comfortable doing... Unless there's a better way :-)
In this case, it's not worth splitting the patch into "implement the macro" and
"use the macro" because both parts are so simple (even though the latter part
is large), so just put it all in a single patch.
If you did want to make two separate diffs against HEAD, it's easy to do so
because the changes are in separate files:
svn diff subversion/include/types.h > unused-param-implement.patch
svn diff subversion/{lib*,clients,svn*}/ > unused-param-use.patch
By the way, the log message for this last part of the change doesn't (in my
opinion) need to have too much detail. Perhaps list the files in which the
macro is used, but not the names of the functions.
I'm still thinking about whether we can do better than putting the definition
in svn_types.h. We really should try hard to avoid using that file as a
dumping ground. I'm thinking svn_private_config.h would be better, since this
is an implementation detail that users of our libraries don't need to know
about, and since the appropriate definition might depend on the compilation
environment. For example, some compilers might complain more about "(void)x"
being a statement with no effect than they do about unused parameters.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Move unused parameter warning suppression into a
macro
Posted by Jonathan Gilbert <o2...@sneakemail.com>.
At 12:21 PM 25/10/2005 +0100, Julian Foad wrote:
>If you mean all the "unused parameter" warnings, then yes; I hope Jonathan
will
>do that.
If you commit the basic patch, then I'll do that. Otherwise, I'll be stuck
with trying to split it into two diffs against HEAD, which I don't feel
comfortable doing... Unless there's a better way :-)
I've already made a list of the places where there are parameters that are
unused.
Jonathan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Move unused parameter warning suppression into a macro
Posted by Marcus Rueckert <da...@web.de>.
On 2005-10-25 06:13:28 -0400, Michael Sinz wrote:
> PS - if someone were to get all warnings out in a single patch, would that
> be something to look at? I assume we are mainly worried about GCC. The
> HPUX C/C++ compiler is the best warning generating compiler that I have
> ever run into but I currently do not have access to such a machine.
try gcc 4.x. especially 4.1.
darix
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Move unused parameter warning suppression into a macro
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
>
> We choose our own level of "cleanliness" by enabling and
> disabling certain warnings. I don't normally see any "unused parameter"
> warnings because I disable them because I know that we don't currently
> attempt to avoid them.
By "we choose our own level" I meant, to a large extent, that the project as a
whole chooses a style of coding, and that style determines which compiler
warnings are useful and which are not.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Move unused parameter warning suppression into a macro
Posted by Julian Foad <ju...@btopenworld.com>.
Michael Sinz wrote:
> Julian Foad wrote:
>
>> Jonathan Gilbert wrote:
>>
>>> Move unused parameter warning suppression into a macro instead of directly
>>> using "(void)p;" or "p=p;" in otherwise semantically-oriented source code.
>>
>> Are you planning to follow up with a patch that inserts this macro
>> into the dozens (from memory) of other places where we intentionally
>> ignore a parameter, so as to make the whole build "clean"? If so, +1;
>> if not, I don't think there is any point in having this and we should
>> just delete those three instances of ad-hoc warning-suppression that
>> you found.
>
> I believe that this is a good patch. Yes, it will be nice to fix all
> of the warnings, but even if it does not do so in one patch does not
> mean that it is a bad idea or a step in the wrong direction.
To be useful, it has to be used. To use it, we have to define this macro, we
have to turn on the "unused parameter" warnings in our builds, and we have to
insert this macro in every place where a parameter is unused. What I am saying
is that there is no benefit in doing part of this if we are not intending to do
all of it.
> (Personally, my own code always must compile clean - no warnings - before
> I release - so any steps to that condition are good, IMHO)
I like a "clean" compile too, but there is no canonical definition of "clean".
We choose our own level of "cleanliness" by enabling and disabling certain
warnings. I don't normally see any "unused parameter" warnings because I
disable them because I know that we don't currently attempt to avoid them.
> PS - if someone were to get all warnings out in a single patch, would that
> be something to look at? I assume we are mainly worried about GCC.
If you mean all the "unused parameter" warnings, then yes; I hope Jonathan will
do that.
If you mean more than that, then you have to choose and specify what warnings
you mean.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Move unused parameter warning suppression into a macro
Posted by Michael Sinz <Mi...@sinz.org>.
Julian Foad wrote:
> Jonathan Gilbert wrote:
>
>>
>> Move unused parameter warning suppression into a macro instead of
>> directly
>> using "(void)p;" or "p=p;" in otherwise semantically-oriented source
>> code.
>
>
> Are you planning to follow up with a patch that inserts this macro into
> the dozens (from memory) of other places where we intentionally ignore a
> parameter, so as to make the whole build "clean"? If so, +1; if not, I
> don't think there is any point in having this and we should just delete
> those three instances of ad-hoc warning-suppression that you found.
I believe that this is a good patch. Yes, it will be nice to fix all
of the warnings, but even if it does not do so in one patch does not
mean that it is a bad idea or a step in the wrong direction.
(Personally, my own code always must compile clean - no warnings - before
I release - so any steps to that condition are good, IMHO)
PS - if someone were to get all warnings out in a single patch, would that
be something to look at? I assume we are mainly worried about GCC. The
HPUX C/C++ compiler is the best warning generating compiler that I have
ever run into but I currently do not have access to such a machine.
--
Michael Sinz Technology and Engineering Director/Consultant
"Starting Startups" mailto:michael.sinz@sinz.org
My place on the web http://www.sinz.org/Michael.Sinz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Move unused parameter warning suppression into a macro
Posted by Julian Foad <ju...@btopenworld.com>.
Jonathan Gilbert wrote:
>
> Move unused parameter warning suppression into a macro instead of directly
> using "(void)p;" or "p=p;" in otherwise semantically-oriented source code.
Are you planning to follow up with a patch that inserts this macro into the
dozens (from memory) of other places where we intentionally ignore a parameter,
so as to make the whole build "clean"? If so, +1; if not, I don't think there
is any point in having this and we should just delete those three instances of
ad-hoc warning-suppression that you found.
- Julian
[...]
> +#define SVN_UNUSED_PARAMETER(x) ((void)x)
[...]
> Index: subversion/libsvn_wc/adm_files.c
[...]
> Index: subversion/libsvn_subr/config.c
[...]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Move unused parameter warning suppression into a macro
Posted by Branko Čibej <br...@xbc.nu>.
Jonathan Gilbert wrote:
>At David James recommendation, I have placed the macro into svn_types.h.
>This seemed the least illogical place to put it, but none of the existing
>headers were really a perfect match. :-)
>
>Log:
>
>Move unused parameter warning suppression into a macro instead of directly
>using "(void)p;" or "p=p;" in otherwise semantically-oriented source code.
>
>Index: subversion/include/svn_types.h
>===================================================================
>--- subversion/include/svn_types.h (revision 16987)
>+++ subversion/include/svn_types.h (working copy)
>@@ -84,6 +84,16 @@
>
> /** @} */
>
>+/** A macro to suppress some compilers' "Parameter is not used" warnings */
>+#ifndef SVN_UNUSED_PARAMETER
>+#define SVN_UNUSED_PARAMETER(x) ((void)x)
>+/* Possible alternative:
>+ *
>+ * #define SVN_UNUSED_PARAMETER(x) ((x) = (x))
>+ */
>+#endif /* SVN_UNUSED_PARAMETER */
>
>
You don't need the #ifndef; we own the SVN_* namespace, and it's
certainly better to get a compiler warning that to have the macro
silently defined to something entirely different. :)
Otherwise, +1.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Move unused parameter warning suppression into a
macro
Posted by Jonathan Gilbert <o2...@sneakemail.com>.
At 12:26 AM 25/10/2005 -0500, I wrote:
>Log:
>
>Move unused parameter warning suppression into a macro instead of directly
>using "(void)p;" or "p=p;" in otherwise semantically-oriented source code.
Complete log:
Move unused parameter warning suppression into a macro.
Source code was directly using "(void)p;" or "p=p;" in otherwise
semantically-oriented source code.
* subversion/include/svn_types.h: Added macro SVN_UNUSED_PARAMETER(x).
* subversion/libsvn_wc/adm_files.c: Changed '(void)pool;' to
'SVN_UNUSED_PARAMETER(pool);' (in two places).
* subversion/libsvn_subr/config.c: Changed '(void)(baton);' and
'(void)(section);' to 'SVN_UNUSED_PARAMETER(baton);' and
'SVN_UNUSED_PARAMETER(section);' respectively.
Sorry about that, I completely forget when I fired off the e-mail. I even
knew about SVN log messages too! :-)
Jonathan Gilbert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org