Re: [PATCHv5 1/2] http.c: implement the GIT_TRACE_CURL environment variable

[Jeff King <peff@peff.net>]

On Mon, May 02, 2016 at 11:59:14AM -0700, Junio C Hamano wrote:

> > +int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp)
> > +{
> > +	const char *text;
> > +	(void)handle;		/* prevent compiler unused parameter warning if checked */
> > +	(void)userp;		/* prevent compiler unused parameter warning if checked */
> 
> I really do not want to see these casts.  Unused parameters are
> perfectly normal in a codebase with callback functions, no?  I do
> not think these are the first occurrences of unused parameters in
> our codebase, and I do not think we have such cast to void to them.
> Why add this ugliness only to here?

Yeah, it is pointless to use -Wunused-parameter in our code base, as it
turns up over 1200 hits (though some are repeats from include files).
Most are callbacks, but some also look like compat functions. But
clearly it's going to be a false positive any time the interface to the
function is dictated by something other than the function body.

I generally don't mind quieting false positive warnings if we think it
might provide an opportunity for cleanup (i.e., parameters that really
_can_ go away). But I don't think gcc provides a great way to do it.

You can mark individual parameters as unused, like:

diff --git a/git-compat-util.h b/git-compat-util.h
index ba51cfd..71b4f7b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -331,7 +331,7 @@ extern char *gitdirname(char *);
 #endif
 
 #ifndef has_dos_drive_prefix
-static inline int git_has_dos_drive_prefix(const char *path)
+static inline int git_has_dos_drive_prefix(const char *path __attribute__((unused)))
 {
 	return 0;
 }
@@ -339,7 +339,7 @@ static inline int git_has_dos_drive_prefix(const char *path)
 #endif
 
 #ifndef skip_dos_drive_prefix
-static inline int git_skip_dos_drive_prefix(char **path)
+static inline int git_skip_dos_drive_prefix(char **path __attribute__((unused)))
 {
 	return 0;
 }

That's not too bad in a single-argument function, but for callbacks it
can get pretty tedious:

diff --git a/wt-status.c b/wt-status.c
index 1ea2ebe..7f00981 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1300,8 +1300,11 @@ struct grab_1st_switch_cbdata {
 	unsigned char nsha1[20];
 };
 
-static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,
-			   const char *email, unsigned long timestamp, int tz,
+static int grab_1st_switch(unsigned char *osha1 __attribute__((unused)),
+			   unsigned char *nsha1,
+			   const char *email __attribute__((unused)),
+			   unsigned long timestamp __attribute__((unused)),
+			   int tz __attribute__((unused)),
 			   const char *message, void *cb_data)
 {
 	struct grab_1st_switch_cbdata *cb = cb_data;

It would be much nicer if we had some way of declaring the whole
_function_ as having an interface dictated elsewhere. Then it becomes a
single attribute per callback function, and actually tells the human
reader something useful: not "I happen to not need these variables right
now", but "my interface is specified elsewhere and not up for debate".

But AFAIK, there's no such attribute.

-Peff