Re: [PATCH 0/11] annotating unused function parameters

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Aug 19 2022, Derrick Stolee wrote:

> On 8/19/2022 9:58 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Fri, Aug 19 2022, Jeff King wrote:
>> 
>>> I've been carrying a bunch of patches (for almost 4 years now!) that get
>>> the code base compiling cleanly with -Wunused-parameter. This is a
>>> useful warning in my opinion; it found real bugs[1] when applied to the
>>> whole code base. So it would be nice to be able to turn it on all the
>>> time and get the same protection going forward.
>>> [...]
>>> And of course the most important question is: do we like this direction
>>> overall. This mass-annotation is a one-time pain. Going forward, the
>>> only work would be requiring people to annotate new functions they add
>>> (which again, is mostly going to be callbacks). IMHO it's worth it. In
>>> addition to possibly finding errors, I think the annotations serve as an
>>> extra clue for people reading the code about what the author intended.
>> 
>> I've known you've had this out-of-tree for a while, and really like that
>> it's on the path to getting integrated.
>> 
>> But I have a hang-up about it, which is that I though __attribute__
>> (unused) didn't work like *that*.
>> 
>> What it means (and maybe only I find this counter-intuitive) is "trust
>> me, this is unused, but don't check!", furthermore it causes the
>> compiler to completely ignore the variable for the purposes of *all*
>> warnings, not just the unused one.
>
> That's not the reason for the attribute at all. It's supposed to say "I
> know this is unused, but I still need it to be in the parameter list for
> other reasons. Don't create a warning for this case."
>
> Interpreting it the way you are means "don't do the analysis. Just throw a
> warning." which doesn't make any sense.
>
>> I may still be missing something, but I wonder if this squashed in
>> wouldn't be much better:
>> 	
>> 	diff --git a/git-compat-util.h b/git-compat-util.h
>> 	index a9690126bb0..e02e2fc3f6d 100644
>> 	--- a/git-compat-util.h
>> 	+++ b/git-compat-util.h
>> 	@@ -190,9 +190,9 @@ struct strbuf;
>> 	 #define _SGI_SOURCE 1
>> 	 
>> 	 #if defined(__GNUC__)
>> 	-#define UNUSED(var) UNUSED_##var __attribute__((unused))
>> 	+#define UNUSED(var) var __attribute__((deprecated ("not 'deprecated', but expected not to be used!")))
>> 	 #else
>> 	-#define UNUSED(var) UNUSED_##var
>> 	+#define UNUSED(var) var
>> 	 #endif
>
> Does the deprecated attribute imply unused? Or at the very least, does it
> avoid the -Wunused-parameter warnings?
>
> It might be helpful to _also_ have a deprecated annotation so we know to
> remove the UNUSED macro if a parameter starts being used again. The
> existing macro changes the variable name so we would get compiler errors
> if we started using it, but we could have a better message indicating
> exactly why things are not working.
>
> So in that sense, you are onto something. Should we use both attributes?
>
> At the very least, the warning message you recommend in the 'deprecated'
> attribute could be more direct about what we expect.
> 	 
> 	 #if defined(__GNUC__)
> 	-#define UNUSED(var) UNUSED_##var __attribute__((unused))
> 	+#define UNUSED(var) var __attribute__((unused)) \
> 				 __attribute__((deprecated ("remove UNUSED macro before using")))
> 	 #else
> 	-#define UNUSED(var) UNUSED_##var
> 	+#define UNUSED(var) var
> 	 #endif
> 	 
> 	 #if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */

Yes, I spoke too soon, sorry. We still need ((unused)). FWIW the below
on top of master and doing:

	 make bisect.o CFLAGS=-Wunused-parameter

Does the right thing for me, and then warns if you uncomment that "//"
commented fprintf().

The reason I was confused is that -Wunused-parameter is *not* needed
with ((deprecated)) to error if the variable is used, but it *is* needed
to hide it from -Wunused-parameter if you're trying to find the next
thing to mark as "UNUSED" (or to refactor away).

I think the below version of it also shows that you don't need to pass
the variable name to the macro. If the only reason for that was to avoid
accidental use it seems like the ((deprecated)) attribute should cover
it.

I'd prefer it if we could avoid the funny syntax, it's highlighted a bit
weirdly in my editor, and I suspect I'm not the only one, but mainly
having the extra compiler-enforced sanity checking of checking if it's
accidentally used, without renaming the variable, which seems like a bit
of a hack.

diff --git a/bisect.c b/bisect.c
index 38b3891f3a6..fd581b85a72 100644
--- a/bisect.c
+++ b/bisect.c
@@ -441,7 +441,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
 }
 
 static int register_ref(const char *refname, const struct object_id *oid,
-			int flags, void *cb_data)
+			int flags UNUSED, void *cb_data UNUSED)
 {
 	struct strbuf good_prefix = STRBUF_INIT;
 	strbuf_addstr(&good_prefix, term_good);
@@ -1160,8 +1160,9 @@ int estimate_bisect_steps(int all)
 	return (e < 3 * x) ? n : n - 1;
 }
 
-static int mark_for_removal(const char *refname, const struct object_id *oid,
-			    int flag, void *cb_data)
+static int mark_for_removal(const char *refname,
+			    const struct object_id *oid UNUSED,
+			    int flag UNUSED, void *cb_data)
 {
 	struct string_list *refs = cb_data;
 	char *ref = xstrfmt("refs/bisect%s", refname);
diff --git a/git-compat-util.h b/git-compat-util.h
index 36a25ae252f..7f7395fc9f7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -189,6 +189,12 @@ struct strbuf;
 #define _NETBSD_SOURCE 1
 #define _SGI_SOURCE 1
 
+#if defined(__GNUC__)
+#define UNUSED __attribute__((unused)) __attribute__((deprecated ("marked with UNUSED")))
+#else
+#define UNUSED
+#endif
+
 #if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */
 # if !defined(_WIN32_WINNT)
 #  define _WIN32_WINNT 0x0600
@@ -302,7 +308,8 @@ typedef unsigned long uintptr_t;
 #ifdef PRECOMPOSE_UNICODE
 #include "compat/precompose_utf8.h"
 #else
-static inline const char *precompose_argv_prefix(int argc, const char **argv, const char *prefix)
+static inline const char *precompose_argv_prefix(int argc UNUSED, const char **argv UNUSED,
+						 const char *prefix)
 {
 	return prefix;
 }
@@ -397,7 +404,8 @@ typedef uintmax_t timestamp_t;
 #endif
 
 #ifndef platform_core_config
-static inline int noop_core_config(const char *var, const char *value, void *cb)
+static inline int noop_core_config(const char *var UNUSED, const char *value UNUSED,
+				   void *cb UNUSED)
 {
 	return 0;
 }
@@ -410,7 +418,7 @@ int lstat_cache_aware_rmdir(const char *path);
 #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 UNUSED)
 {
 	return 0;
 }
@@ -418,7 +426,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 UNUSED)
 {
 	return 0;
 }
@@ -490,11 +498,13 @@ static inline void extract_id_from_env(const char *env, uid_t *id)
 	}
 }
 
-static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report)
+static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report UNUSED)
 {
 	struct stat st;
 	uid_t euid;
 
+	//fprintf(stderr, "%p", (void*)report);
+
 	if (lstat(path, &st))
 		return 0;
 
diff --git a/object-store.h b/object-store.h
index 5222ee54600..218ffe73604 100644
--- a/object-store.h
+++ b/object-store.h
@@ -141,7 +141,7 @@ struct packed_git {
 
 struct multi_pack_index;
 
-static inline int pack_map_entry_cmp(const void *unused_cmp_data,
+static inline int pack_map_entry_cmp(const void *unused_cmp_data UNUSED,
 				     const struct hashmap_entry *entry,
 				     const struct hashmap_entry *entry2,
 				     const void *keydata)