git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Aug 19, 2022, 20:58 UTC
Message-ID
<220819.868rnk54ju.gmgdl@evledraar.gmail.com>
In-Reply-To
<c22a8317-7d43-d84b-f63f-df2da31b4658@github.com>
On Fri, Aug 19 2022, Derrick Stolee wrote:
Show 77 quoted lines
> 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)
Previous: Derrick StoleeNext: Jeff King
Message 15 of 22 in “annotating unused function parameters”
  1. 0/11 annotating unused function parametersJeff King, Aug 19, 2022
  2. 01/11 git-compat-util: add UNUSED macroJeff King, Aug 19, 2022
  3. 02/11 refs: mark unused each_ref_fn parametersJeff King, Aug 19, 2022
  4. 03/11 refs: mark unused reflog callback parametersJeff King, Aug 19, 2022
  5. 04/11 refs: mark unused virtual method parametersJeff King, Aug 19, 2022
  6. 05/11 transport: mark bundle transport_options as unusedJeff King, Aug 19, 2022
  7. 06/11 streaming: mark unused virtual method parametersJeff King, Aug 19, 2022
  8. 07/11 config: mark unused callback parametersJeff King, Aug 19, 2022
  9. 08/11 hashmap: mark unused callback parametersJeff King, Aug 19, 2022
  10. 09/11 mark unused read_tree_recursive() callback parametersJeff King, Aug 19, 2022
  11. 10/11 run-command: mark unused async callback parametersJeff King, Aug 19, 2022
  12. 11/11 is_path_owned_by_current_uid(): mark "report" parameter as unusedJeff King, Aug 19, 2022
  13. Ævar Arnfjörð BjarmasonAug 19, 2022
  14. Derrick StoleeAug 19, 2022
  15. Ævar Arnfjörð BjarmasonAug 19, 2022
  16. Jeff KingAug 20, 2022
  17. Junio C HamanoAug 20, 2022
  18. Derrick StoleeAug 22, 2022
  19. Ævar Arnfjörð BjarmasonAug 25, 2022
  20. Jeff KingAug 26, 2022
  21. Phillip WoodAug 26, 2022
  22. Junio C HamanoAug 26, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.