{"thread":{"id":"62013","subject":"[PATCH 0/6] unused parameters: the final countdown","startedAt":"2024-08-28T03:57:30Z","lastAt":"2024-08-28T19:54:04Z","messageCount":13,"participants":["Jeff King","Eric Sunshine","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"501744","messageId":"20240828035722.GA3998881@coredump.intra.peff.net","threadId":"62013","inReplyTo":null,"subject":"[PATCH 0/6] unused parameters: the final countdown","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-28T03:57:22Z","receivedAt":"2024-08-28T03:57:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After many long sets of patches fixing and annotating existing cases,\nthis is the culminating series that actually turns on -Wunused-parameter\nin our DEVELOPER=1 builds.\n\nWhen applied on 'master', everything should compile cleanly with the new\nwarning. There are some new cases introduced in 'next', but I'll send\nseparate patches to go on those individual topics.\n\nThe first two patches fix new spots that cropped up since the last round\nof fixes. Patches 3-5 address compat/ code. And then the interesting one\nis patch 6.\n\n [1/6]: gc: mark unused config parameter in virtual functions\n [2/6]: t-reftable-readwrite: mark unused parameter in callback function\n [3/6]: compat: disable -Wunused-parameter in 3rd-party code\n [4/6]: compat: disable -Wunused-parameter in win32/headless.c\n [5/6]: compat: mark unused parameters in win32/mingw functions\n [6/6]: config.mak.dev: enable -Wunused-parameter by default\n\n builtin/gc.c | 16 ++++++++--------\n compat/mingw.c | 15 ++++++++-------\n compat/mingw.h | 18 +++++++++---------\n compat/nedmalloc/nedmalloc.c | 2 ++\n compat/regex/regcomp.c | 2 ++\n compat/stub/procinfo.c | 2 +-\n compat/win32/headless.c | 2 ++\n compat/win32/pthread.c | 2 +-\n compat/win32/pthread.h | 4 ++--\n compat/win32/syslog.c | 2 +-\n compat/win32mmap.c | 2 +-\n compat/winansi.c | 2 +-\n config.mak.dev | 1 -\n t/unit-tests/t-reftable-readwrite.c | 2 +-\n 14 files changed, 39 insertions(+), 33 deletions(-)\n\n-Peff\n"},{"id":"501745","messageId":"20240828035746.GA3999193@coredump.intra.peff.net","threadId":"62013","inReplyTo":"20240828035722.GA3998881@coredump.intra.peff.net","subject":"[PATCH 1/6] gc: mark unused config parameter in virtual functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-28T03:57:46Z","receivedAt":"2024-08-28T03:57:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit d1ae15d68b (builtin/gc: refactor to read config into structure,\n2024-08-16) added a new parameter to the maintenance_task virtual\nfunctions, but most of them don't need to look at it.\n\nSigned-off-by: Jeff King \n---\n builtin/gc.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 427faf1cfe..0e361253e3 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -967,7 +967,7 @@ static int dfs_on_ref(const char *refname UNUSED,\n \treturn result;\n }\n \n-static int should_write_commit_graph(struct gc_config *cfg)\n+static int should_write_commit_graph(struct gc_config *cfg UNUSED)\n {\n \tint result;\n \tstruct cg_auto_data data;\n@@ -1005,7 +1005,7 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts)\n }\n \n static int maintenance_task_commit_graph(struct maintenance_run_opts *opts,\n-\t\t\t\t\t struct gc_config *cfg)\n+\t\t\t\t\t struct gc_config *cfg UNUSED)\n {\n \tprepare_repo_settings(the_repository);\n \tif (!the_repository->settings.core_commit_graph)\n@@ -1040,7 +1040,7 @@ static int fetch_remote(struct remote *remote, void *cbdata)\n }\n \n static int maintenance_task_prefetch(struct maintenance_run_opts *opts,\n-\t\t\t\t struct gc_config *cfg)\n+\t\t\t\t struct gc_config *cfg UNUSED)\n {\n \tif (for_each_remote(fetch_remote, opts)) {\n \t\terror(_(\"failed to prefetch remotes\"));\n@@ -1051,7 +1051,7 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts,\n }\n \n static int maintenance_task_gc(struct maintenance_run_opts *opts,\n-\t\t\t struct gc_config *cfg)\n+\t\t\t struct gc_config *cfg UNUSED)\n {\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \n@@ -1100,7 +1100,7 @@ static int loose_object_count(const struct object_id *oid UNUSED,\n \treturn 0;\n }\n \n-static int loose_object_auto_condition(struct gc_config *cfg)\n+static int loose_object_auto_condition(struct gc_config *cfg UNUSED)\n {\n \tint count = 0;\n \n@@ -1192,12 +1192,12 @@ static int pack_loose(struct maintenance_run_opts *opts)\n }\n \n static int maintenance_task_loose_objects(struct maintenance_run_opts *opts,\n-\t\t\t\t\t struct gc_config *cfg)\n+\t\t\t\t\t struct gc_config *cfg UNUSED)\n {\n \treturn prune_packed(opts) || pack_loose(opts);\n }\n \n-static int incremental_repack_auto_condition(struct gc_config *cfg)\n+static int incremental_repack_auto_condition(struct gc_config *cfg UNUSED)\n {\n \tstruct packed_git *p;\n \tint incremental_repack_auto_limit = 10;\n@@ -1317,7 +1317,7 @@ static int multi_pack_index_repack(struct maintenance_run_opts *opts)\n }\n \n static int maintenance_task_incremental_repack(struct maintenance_run_opts *opts,\n-\t\t\t\t\t struct gc_config *cfg)\n+\t\t\t\t\t struct gc_config *cfg UNUSED)\n {\n \tprepare_repo_settings(the_repository);\n \tif (!the_repository->settings.core_multi_pack_index) {\n-- \n2.46.0.754.g24c813f009\n\n"},{"id":"501746","messageId":"20240828035758.GB3999193@coredump.intra.peff.net","threadId":"62013","inReplyTo":"20240828035722.GA3998881@coredump.intra.peff.net","subject":"[PATCH 2/6] t-reftable-readwrite: mark unused parameter in callback function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-28T03:57:58Z","receivedAt":"2024-08-28T03:57:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This spot was originally marked in in 4695c3f3a9 (reftable: mark unused\nparameters in virtual functions, 2024-08-17), but was copied in\n5b539a5361 (t: move reftable/readwrite_test.c to the unit testing\nframework, 2024-08-13).\n\nSigned-off-by: Jeff King \n---\n t/unit-tests/t-reftable-readwrite.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex 1eae36cc60..178296891c 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -26,7 +26,7 @@ static ssize_t strbuf_add_void(void *b, const void *data, size_t sz)\n \treturn sz;\n }\n \n-static int noop_flush(void *arg)\n+static int noop_flush(void *arg UNUSED)\n {\n \treturn 0;\n }\n-- \n2.46.0.754.g24c813f009\n\n"},{"id":"501747","messageId":"20240828035855.GC3999193@coredump.intra.peff.net","threadId":"62013","inReplyTo":"20240828035722.GA3998881@coredump.intra.peff.net","subject":"[PATCH 3/6] compat: disable -Wunused-parameter in 3rd-party code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-28T03:58:55Z","receivedAt":"2024-08-28T03:58:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We carry some vendored 3rd-party code in compat/ that does not build\ncleanly with -Wunused-parameters. We could mark these with UNUSED, but\nthere are two reasons not to:\n\n 1. This is code imported from elsewhere, so we'd prefer to avoid\n modifying it in an invasive way that could create conflicts if we\n tried to pull in a new version.\n\n 2. These files don't include git-compat-util.h at all, so we'd need to\n factor out (or repeat) our UNUSED macro.\n\nIn theory we could modify the build process to invoke the compiler with\nthe extra warning disabled for these files, but there are tricky corner\ncases there (e.g., for NO_REGEX we cannot assume that the compiler\nunderstands -Wno-unused-parameter as an option, so we'd have to use our\ndetect-compiler script).\n\nInstead, let's rely on the gcc diagnostic #pragma. This is horribly\nunportable, of course, but it should do what we want. Compilers which\ndon't understand this particular pragma should ignore it (per the\nstandard), and compilers which do care about \"-Wunused-parameter\" will\nhopefully respect it, even if they are not gcc (e.g., clang does).\n\nSigned-off-by: Jeff King \n---\n compat/nedmalloc/nedmalloc.c | 2 ++\n compat/regex/regcomp.c | 2 ++\n 2 files changed, 4 insertions(+)\n\ndiff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c\nindex 2c0ace7075..145255da43 100644\n--- a/compat/nedmalloc/nedmalloc.c\n+++ b/compat/nedmalloc/nedmalloc.c\n@@ -31,6 +31,8 @@ DEALINGS IN THE SOFTWARE.\n /*#pragma optimize(\"a\", on)*/\n #endif\n \n+#pragma GCC diagnostic ignored \"-Wunused-parameter\"\n+\n /*#define FULLSANITYCHECKS*/\n \n #include \"nedmalloc.h\"\ndiff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c\nindex 6c5d455e92..8d93a9b93f 100644\n--- a/compat/regex/regcomp.c\n+++ b/compat/regex/regcomp.c\n@@ -17,6 +17,8 @@\n License along with the GNU C Library; if not, see\n . */\n \n+#pragma GCC diagnostic ignored \"-Wunused-parameter\"\n+\n #if defined __TANDEM\n /* This is currently duplicated from git-compat-utils.h */\n # ifdef NO_INTPTR_T\n-- \n2.46.0.754.g24c813f009\n\n"},{"id":"501748","messageId":"20240828035952.GD3999193@coredump.intra.peff.net","threadId":"62013","inReplyTo":"20240828035722.GA3998881@coredump.intra.peff.net","subject":"[PATCH 4/6] compat: disable -Wunused-parameter in win32/headless.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-28T03:59:52Z","receivedAt":"2024-08-28T03:59:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As with the files touched in the previous commit, win32/headless.c does\nnot include git-compat-util.h, so it doesn't have our UNUSED macro.\nUnlike those ones, this is not third-party code, so it would not be a\nbig deal to modify it.\n\nHowever, I'm not sure if including git-compat-util.h would create other\nheadaches (and I don't even have a machine to test this on; I'm relying\non Windows CI to compile it at all). Given how trivial the file is, and\nthat the unused parameters are not interesting (they are just\nboilerplate for the wWinMain() function), we can just use the same trick\nas the previous commit and disable the warnings via pragma.\n\nSigned-off-by: Jeff King \n---\n compat/win32/headless.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/compat/win32/headless.c b/compat/win32/headless.c\nindex 8b00dfe3bd..11392a0b9a 100644\n--- a/compat/win32/headless.c\n+++ b/compat/win32/headless.c\n@@ -11,6 +11,8 @@\n #include \n #include \n \n+#pragma GCC diagnostic ignored \"-Wunused-parameter\"\n+\n /*\n * If `dir` contains the path to a Git exec directory, extend `PATH` to\n * include the corresponding `bin/` directory (which is where all those\n-- \n2.46.0.754.g24c813f009\n\n"},{"id":"501749","messageId":"20240828040016.GE3999193@coredump.intra.peff.net","threadId":"62013","inReplyTo":"20240828035722.GA3998881@coredump.intra.peff.net","subject":"[PATCH 5/6] compat: mark unused parameters in win32/mingw functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-28T04:00:16Z","receivedAt":"2024-08-28T04:00:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The compat/ directory contains many stub functions, wrappers, and so on\nthat have to conform to a specific interface, but don't necessarily need\nto use all of their parameters. Let's mark them to avoid complaints from\n-Wunused-parameter.\n\nThis was done mostly via guess-and-check with the Windows build in\nGitHub CI. I also confirmed that the win+VS build is similarly happy.\n\nSigned-off-by: Jeff King \n---\n compat/mingw.c | 15 ++++++++-------\n compat/mingw.h | 18 +++++++++---------\n compat/stub/procinfo.c | 2 +-\n compat/win32/pthread.c | 2 +-\n compat/win32/pthread.h | 4 ++--\n compat/win32/syslog.c | 2 +-\n compat/win32mmap.c | 2 +-\n compat/winansi.c | 2 +-\n 8 files changed, 24 insertions(+), 23 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 29d3f09768..eb13c02a76 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -243,7 +243,8 @@ static enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY;\n static char *unset_environment_variables;\n \n int mingw_core_config(const char *var, const char *value,\n-\t\t const struct config_context *ctx, void *cb)\n+\t\t const struct config_context *ctx UNUSED,\n+\t\t void *cb UNUSED)\n {\n \tif (!strcmp(var, \"core.hidedotfiles\")) {\n \t\tif (value && !strcasecmp(value, \"dotgitonly\"))\n@@ -453,7 +454,7 @@ static int set_hidden_flag(const wchar_t *path, int set)\n \treturn -1;\n }\n \n-int mingw_mkdir(const char *path, int mode)\n+int mingw_mkdir(const char *path, int mode UNUSED)\n {\n \tint ret;\n \twchar_t wpath[MAX_PATH];\n@@ -597,7 +598,7 @@ int mingw_open (const char *filename, int oflags, ...)\n \treturn fd;\n }\n \n-static BOOL WINAPI ctrl_ignore(DWORD type)\n+static BOOL WINAPI ctrl_ignore(DWORD type UNUSED)\n {\n \treturn TRUE;\n }\n@@ -1085,7 +1086,7 @@ int mkstemp(char *template)\n \treturn git_mkstemp_mode(template, 0600);\n }\n \n-int gettimeofday(struct timeval *tv, void *tz)\n+int gettimeofday(struct timeval *tv, void *tz UNUSED)\n {\n \tFILETIME ft;\n \tlong long hnsec;\n@@ -2252,7 +2253,7 @@ char *mingw_query_user_email(void)\n \treturn get_extended_user_info(NameUserPrincipal);\n }\n \n-struct passwd *getpwuid(int uid)\n+struct passwd *getpwuid(int uid UNUSED)\n {\n \tstatic unsigned initialized;\n \tstatic char user_name[100];\n@@ -2304,7 +2305,7 @@ static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL;\n * length to call the signal handler.\n */\n \n-static unsigned __stdcall ticktack(void *dummy)\n+static unsigned __stdcall ticktack(void *dummy UNUSED)\n {\n \twhile (WaitForSingleObject(timer_event, timer_interval) == WAIT_TIMEOUT) {\n \t\tmingw_raise(SIGALRM);\n@@ -2352,7 +2353,7 @@ static inline int is_timeval_eq(const struct timeval *i1, const struct timeval *\n \treturn i1->tv_sec == i2->tv_sec && i1->tv_usec == i2->tv_usec;\n }\n \n-int setitimer(int type, struct itimerval *in, struct itimerval *out)\n+int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out)\n {\n \tstatic const struct timeval zero;\n \tstatic int atexit_done;\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 27b61284f4..ebfb8ba423 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -122,27 +122,27 @@ struct utsname {\n * trivial stubs\n */\n \n-static inline int readlink(const char *path, char *buf, size_t bufsiz)\n+static inline int readlink(const char *path UNUSED, char *buf UNUSED, size_t bufsiz UNUSED)\n { errno = ENOSYS; return -1; }\n-static inline int symlink(const char *oldpath, const char *newpath)\n+static inline int symlink(const char *oldpath UNUSED, const char *newpath UNUSED)\n { errno = ENOSYS; return -1; }\n-static inline int fchmod(int fildes, mode_t mode)\n+static inline int fchmod(int fildes UNUSED, mode_t mode UNUSED)\n { errno = ENOSYS; return -1; }\n #ifndef __MINGW64_VERSION_MAJOR\n static inline pid_t fork(void)\n { errno = ENOSYS; return -1; }\n #endif\n-static inline unsigned int alarm(unsigned int seconds)\n+static inline unsigned int alarm(unsigned int seconds UNUSED)\n { return 0; }\n static inline int fsync(int fd)\n { return _commit(fd); }\n static inline void sync(void)\n {}\n static inline uid_t getuid(void)\n { return 1; }\n-static inline struct passwd *getpwnam(const char *name)\n+static inline struct passwd *getpwnam(const char *name UNUSED)\n { return NULL; }\n-static inline int fcntl(int fd, int cmd, ...)\n+static inline int fcntl(int fd UNUSED, int cmd, ...)\n {\n \tif (cmd == F_GETFD || cmd == F_SETFD)\n \t\treturn 0;\n@@ -151,17 +151,17 @@ static inline int fcntl(int fd, int cmd, ...)\n }\n \n #define sigemptyset(x) (void)0\n-static inline int sigaddset(sigset_t *set, int signum)\n+static inline int sigaddset(sigset_t *set UNUSED, int signum UNUSED)\n { return 0; }\n #define SIG_BLOCK 0\n #define SIG_UNBLOCK 0\n-static inline int sigprocmask(int how, const sigset_t *set, sigset_t *oldset)\n+static inline int sigprocmask(int how UNUSED, const sigset_t *set UNUSED, sigset_t *oldset UNUSED)\n { return 0; }\n static inline pid_t getppid(void)\n { return 1; }\n static inline pid_t getpgid(pid_t pid)\n { return pid == 0 ? getpid() : pid; }\n-static inline pid_t tcgetpgrp(int fd)\n+static inline pid_t tcgetpgrp(int fd UNUSED)\n { return getpid(); }\n \n /*\ndiff --git a/compat/stub/procinfo.c b/compat/stub/procinfo.c\nindex 12c0a23c9e..3168cd5714 100644\n--- a/compat/stub/procinfo.c\n+++ b/compat/stub/procinfo.c\n@@ -6,6 +6,6 @@\n * Stub. See sample implementations in compat/linux/procinfo.c and\n * compat/win32/trace2_win32_process_info.c.\n */\n-void trace2_collect_process_info(enum trace2_process_info_reason reason)\n+void trace2_collect_process_info(enum trace2_process_info_reason reason UNUSED)\n {\n }\ndiff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\nindex 85f8f7920c..58980a529c 100644\n--- a/compat/win32/pthread.c\n+++ b/compat/win32/pthread.c\n@@ -21,7 +21,7 @@ static unsigned __stdcall win32_start_routine(void *arg)\n \treturn 0;\n }\n \n-int pthread_create(pthread_t *thread, const void *unused,\n+int pthread_create(pthread_t *thread, const void *attr UNUSED,\n \t\t void *(*start_routine)(void *), void *arg)\n {\n \tthread->arg = arg;\ndiff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\nindex cc3221cb2c..e2b5c4f64c 100644\n--- a/compat/win32/pthread.h\n+++ b/compat/win32/pthread.h\n@@ -18,7 +18,7 @@\n */\n #define pthread_mutex_t CRITICAL_SECTION\n \n-static inline int return_0(int i) {\n+static inline int return_0(int i UNUSED) {\n \treturn 0;\n }\n #define pthread_mutex_init(a,b) return_0((InitializeCriticalSection((a)), 0))\n@@ -70,7 +70,7 @@ static inline void NORETURN pthread_exit(void *ret)\n }\n \n typedef DWORD pthread_key_t;\n-static inline int pthread_key_create(pthread_key_t *keyp, void (*destructor)(void *value))\n+static inline int pthread_key_create(pthread_key_t *keyp, void (*destructor)(void *value) UNUSED)\n {\n \treturn (*keyp = TlsAlloc()) == TLS_OUT_OF_INDEXES ? EAGAIN : 0;\n }\ndiff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\nindex 0af18d8882..4e4794743a 100644\n--- a/compat/win32/syslog.c\n+++ b/compat/win32/syslog.c\n@@ -2,7 +2,7 @@\n \n static HANDLE ms_eventlog;\n \n-void openlog(const char *ident, int logopt, int facility)\n+void openlog(const char *ident, int logopt UNUSED, int facility UNUSED)\n {\n \tif (ms_eventlog)\n \t\treturn;\ndiff --git a/compat/win32mmap.c b/compat/win32mmap.c\nindex 519d51f2b6..a4ab4cb939 100644\n--- a/compat/win32mmap.c\n+++ b/compat/win32mmap.c\n@@ -40,7 +40,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \treturn MAP_FAILED;\n }\n \n-int git_munmap(void *start, size_t length)\n+int git_munmap(void *start, size_t length UNUSED)\n {\n \treturn !UnmapViewOfFile(start);\n }\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex 575813bde8..1b3f916b9f 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -340,7 +340,7 @@ enum {\n \tTEXT = 0, ESCAPE = 033, BRACKET = '['\n };\n \n-static DWORD WINAPI console_thread(LPVOID unused)\n+static DWORD WINAPI console_thread(LPVOID data UNUSED)\n {\n \tunsigned char buffer[BUFFER_SIZE];\n \tDWORD bytes;\n-- \n2.46.0.754.g24c813f009\n\n"},{"id":"501750","messageId":"20240828040049.GF3999193@coredump.intra.peff.net","threadId":"62013","inReplyTo":"20240828035722.GA3998881@coredump.intra.peff.net","subject":"[PATCH 6/6] config.mak.dev: enable -Wunused-parameter by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-28T04:00:49Z","receivedAt":"2024-08-28T04:00:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Having now removed or annotated all of the unused function parameters in\nour code base, I found that each instance falls into one of three\ncategories:\n\n 1. ignoring the parameter is a bug (e.g., a function takes a ptr/len\n pair, but ignores the length). Detecting these helps us find the\n bugs.\n\n 2. the parameter is unnecessary (and usually left over from a\n refactoring or earlier iteration of a patches series). Removing\n these cleans up the code.\n\n 3. the function has to conform to a specific interface (because it's\n used via a function pointer, or matches something on the other side\n of an #ifdef). These ones are annoying, but annotating them with\n UNUSED is not too bad (especially if the compiler tells you about\n the problem promptly).\n\nCertainly instances of (3) are more common than (1), but after finding\nall of these, I think there were enough cases of (1) that it justifies\nthe work in annotating all of the (3)s.\n\nAnd since the code base is now at a spot where we compile cleanly with\n-Wunused-parameter, turning it on will make it the responsibility of\nindividual patch writers going forward.\n\nSigned-off-by: Jeff King \n---\n config.mak.dev | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 5229c35484..50026d1e0e 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -54,7 +54,6 @@ ifeq ($(filter extra-all,$(DEVOPTS)),)\n DEVELOPER_CFLAGS += -Wno-empty-body\n DEVELOPER_CFLAGS += -Wno-missing-field-initializers\n DEVELOPER_CFLAGS += -Wno-sign-compare\n-DEVELOPER_CFLAGS += -Wno-unused-parameter\n endif\n endif\n \n-- \n2.46.0.754.g24c813f009\n"},{"id":"501753","messageId":"20240828041252.GA4009884@coredump.intra.peff.net","threadId":"62013","inReplyTo":"20240828035722.GA3998881@coredump.intra.peff.net","subject":"Re: [PATCH 0/6] unused parameters: the final countdown","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-28T04:12:52Z","receivedAt":"2024-08-28T04:12:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 27, 2024 at 11:57:23PM -0400, Jeff King wrote:\n\n> When applied on 'master', everything should compile cleanly with the new\n> warning. There are some new cases introduced in 'next', but I'll send\n> separate patches to go on those individual topics.\n\nThose are at:\n\n https://lore.kernel.org/git/20240828040944.GA4005021@coredump.intra.peff.net/\n\nand:\n\n https://lore.kernel.org/git/20240828040803.GA4004932@coredump.intra.peff.net/\n\nI double-checked against what's in 'seen', and there's nothing new\nthere. Of course I won't be surprised if something new pops up while\nthis topic is cooking, but CI will start catching it in any branch where\nthis is merged (and the solution is usually pretty obvious).\n\n-Peff\n"},{"id":"501754","messageId":"CAPig+cQLr+vAzkt8UJNVCeE8osGEcEfFunG36oqxa0k8JamJzQ@mail.gmail.com","threadId":"62013","inReplyTo":"20240828040049.GF3999193@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] config.mak.dev: enable -Wunused-parameter by default","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-28T05:56:13Z","receivedAt":"2024-08-28T05:56:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 28, 2024 at 12:01 AM Jeff King wrote:\n> Having now removed or annotated all of the unused function parameters in\n> our code base, I found that each instance falls into one of three\n> categories:\n>\n> 1. ignoring the parameter is a bug (e.g., a function takes a ptr/len\n> pair, but ignores the length). Detecting these helps us find the\n> bugs.\n>\n> 2. the parameter is unnecessary (and usually left over from a\n> refactoring or earlier iteration of a patches series). Removing\n> these cleans up the code.\n>\n> 3. the function has to conform to a specific interface (because it's\n> used via a function pointer, or matches something on the other side\n> of an #ifdef). These ones are annoying, but annotating them with\n> UNUSED is not too bad (especially if the compiler tells you about\n> the problem promptly).\n> [...]\n> And since the code base is now at a spot where we compile cleanly with\n> -Wunused-parameter, turning it on will make it the responsibility of\n> individual patch writers going forward.\n>\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/config.mak.dev b/config.mak.dev\n> @@ -54,7 +54,6 @@ ifeq ($(filter extra-all,$(DEVOPTS)),)\n> DEVELOPER_CFLAGS += -Wno-sign-compare\n> -DEVELOPER_CFLAGS += -Wno-unused-parameter\n\nWhat is the expectation regarding newcomers to the project or even\npeople who have not been following this topic and its cousins?\nDocumentation/CodingGuidelines recommends enabling DEVELOPER mode,\nwhich is good, but this change means that such people may now be hit\nwith a compiler complaint which they don't necessarily know how to\ndeal with in the legitimate case #3 (described above). Should\nCodingGuidelines be updated to mention \"UNUSED\" and the circumstances\nunder which it should be used?\n"},{"id":"501756","messageId":"Zs7eBocE3RZb71-q@tanuki","threadId":"62013","inReplyTo":"CAPig+cQLr+vAzkt8UJNVCeE8osGEcEfFunG36oqxa0k8JamJzQ@mail.gmail.com","subject":"Re: [PATCH 6/6] config.mak.dev: enable -Wunused-parameter by default","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-28T08:21:26Z","receivedAt":"2024-08-28T08:21:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 28, 2024 at 01:56:13AM -0400, Eric Sunshine wrote:\n> On Wed, Aug 28, 2024 at 12:01 AM Jeff King wrote:\n> > Having now removed or annotated all of the unused function parameters in\n> > our code base, I found that each instance falls into one of three\n> > categories:\n> >\n> > 1. ignoring the parameter is a bug (e.g., a function takes a ptr/len\n> > pair, but ignores the length). Detecting these helps us find the\n> > bugs.\n> >\n> > 2. the parameter is unnecessary (and usually left over from a\n> > refactoring or earlier iteration of a patches series). Removing\n> > these cleans up the code.\n> >\n> > 3. the function has to conform to a specific interface (because it's\n> > used via a function pointer, or matches something on the other side\n> > of an #ifdef). These ones are annoying, but annotating them with\n> > UNUSED is not too bad (especially if the compiler tells you about\n> > the problem promptly).\n> > [...]\n> > And since the code base is now at a spot where we compile cleanly with\n> > -Wunused-parameter, turning it on will make it the responsibility of\n> > individual patch writers going forward.\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> > diff --git a/config.mak.dev b/config.mak.dev\n> > @@ -54,7 +54,6 @@ ifeq ($(filter extra-all,$(DEVOPTS)),)\n> > DEVELOPER_CFLAGS += -Wno-sign-compare\n> > -DEVELOPER_CFLAGS += -Wno-unused-parameter\n> \n> What is the expectation regarding newcomers to the project or even\n> people who have not been following this topic and its cousins?\n> Documentation/CodingGuidelines recommends enabling DEVELOPER mode,\n> which is good, but this change means that such people may now be hit\n> with a compiler complaint which they don't necessarily know how to\n> deal with in the legitimate case #3 (described above). Should\n> CodingGuidelines be updated to mention \"UNUSED\" and the circumstances\n> under which it should be used?\n\nUpdating our coding guidelines would certainly be welcome. Other than\nthat this series looks good to me and is a step into the right direction\nin my opinion. Thanks!\n\nPatrick\n"},{"id":"501771","messageId":"20240828144814.GB4020916@coredump.intra.peff.net","threadId":"62013","inReplyTo":"CAPig+cQLr+vAzkt8UJNVCeE8osGEcEfFunG36oqxa0k8JamJzQ@mail.gmail.com","subject":"[PATCH 7/6] CodingGuidelines: mention -Wunused-parameter and UNUSED","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-28T14:48:14Z","receivedAt":"2024-08-28T14:48:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 28, 2024 at 01:56:13AM -0400, Eric Sunshine wrote:\n\n> > And since the code base is now at a spot where we compile cleanly with\n> > -Wunused-parameter, turning it on will make it the responsibility of\n> > individual patch writers going forward.\n> [...]\n> What is the expectation regarding newcomers to the project or even\n> people who have not been following this topic and its cousins?\n> Documentation/CodingGuidelines recommends enabling DEVELOPER mode,\n> which is good, but this change means that such people may now be hit\n> with a compiler complaint which they don't necessarily know how to\n> deal with in the legitimate case #3 (described above). Should\n> CodingGuidelines be updated to mention \"UNUSED\" and the circumstances\n> under which it should be used?\n\nYeah, I agree some guidance is in order. How about this on top? I didn't\ngo into the decision tree of when you should remove the parameter versus\nusing it versus annotating it. I think in general that the first two are\npretty obvious when they are appropriate, and we just need to focus on\nthe annotating option.\n\n-- >8 --\nSubject: [PATCH] CodingGuidelines: mention -Wunused-parameter and UNUSED\n\nNow that -Wunused-parameter is on by default for DEVELOPER=1 builds,\npeople may trigger it, blocking their build. When it's a mistake for the\nparameter to exist, the path forward is obvious: remove it. But\nsometimes you need to suppress the warning, and the \"UNUSED\" mechanism\nfor that is specific to our project, so people may not know about it.\n\nLet's put some advice in CodingGuidelines, including an example warning\nmessage. That should help people who grep for the warning text after\nseeing it from the compiler.\n\nSigned-off-by: Jeff King \n---\n Documentation/CodingGuidelines | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex ccaea39752..d0fc7cfe60 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -258,6 +258,13 @@ For C programs:\n ensure your patch is clear of all compiler warnings we care about,\n by e.g. \"echo DEVELOPER=1 >>config.mak\".\n \n+ - When using DEVELOPER=1 mode, you may see warnings from the compiler\n+ like \"error: unused parameter 'foo' [-Werror=unused-parameter]\",\n+ which indicates that a function ignores its argument. If the unused\n+ parameter can't be removed (e.g., because the function is used as a\n+ callback and has to match a certain interface), you can annotate the\n+ individual parameters with the UNUSED keyword, like \"int foo UNUSED\".\n+\n - We try to support a wide range of C compilers to compile Git with,\n including old ones. As of Git v2.35.0 Git requires C99 (we check\n \"__STDC_VERSION__\"). You should not use features from a newer C\n-- \n2.46.0.754.g24c813f009\n\n"},{"id":"501774","messageId":"xmqqle0g4qxp.fsf@gitster.g","threadId":"62013","inReplyTo":"CAPig+cQLr+vAzkt8UJNVCeE8osGEcEfFunG36oqxa0k8JamJzQ@mail.gmail.com","subject":"Re: [PATCH 6/6] config.mak.dev: enable -Wunused-parameter by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-28T15:17:54Z","receivedAt":"2024-08-28T15:18:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> 3. the function has to conform to a specific interface (because it's\n>> used via a function pointer, or matches something on the other side\n>> of an #ifdef). These ones are annoying, but annotating them with\n>> UNUSED is not too bad (especially if the compiler tells you about\n>> the problem promptly).\n>> [...]\n>> And since the code base is now at a spot where we compile cleanly with\n>> -Wunused-parameter, turning it on will make it the responsibility of\n>> individual patch writers going forward.\n>>\n>> Signed-off-by: Jeff King \n>> ---\n>> diff --git a/config.mak.dev b/config.mak.dev\n>> @@ -54,7 +54,6 @@ ifeq ($(filter extra-all,$(DEVOPTS)),)\n>> DEVELOPER_CFLAGS += -Wno-sign-compare\n>> -DEVELOPER_CFLAGS += -Wno-unused-parameter\n>\n> What is the expectation regarding newcomers to the project or even\n> people who have not been following this topic and its cousins?\n> Documentation/CodingGuidelines recommends enabling DEVELOPER mode,\n> which is good, but this change means that such people may now be hit\n> with a compiler complaint which they don't necessarily know how to\n> deal with in the legitimate case #3 (described above). Should\n> CodingGuidelines be updated to mention \"UNUSED\" and the circumstances\n> under which it should be used?\n\nI am not yet convinced 100%, but probably it is a good idea.\n\nWe have our idioms and conventions like use of UNLEAK(), UNUSED,\n__attribute__ to annotate varargs functions that take printf-like\nformat, always using \"{ 0 }\" and no other form to zero-initialize an\nauto variable of an aggregate type, etc., that are unreasonable for\nsomebody, who is new to the project but is fluent in and competent\nat C, to know all of them.\n\nFor things like UNLEAK() that require a bit more than general\ncompetence in the language, a bit more thought and peeking the\nimplementation to understand how they work, we should document them\nto make it easier to learn for new people. \n\nConsistently using \"{ 0 }\", instead of picking a single random\nmember and initialize the aggrevate with \"{ .it = 0 }\", is asking\nthe writer to pick between two _valid_ C language constructs and\nalways use one of them, so it may make sense to document it, even\nthough the general \"do what the existing code does\" may be\nsufficient.\n\nUnlike them, UNUSED smells a lot more obvious, and because the\ncode base is full of API functions that take callback functions,\nwe have plenty of existing uses of it for those who are competent\nbut are unfamiliar to the code base to notice and pick up quickly.\n\nSo I have a feeling that relatively speaking it is less necessary to\nhelp new contributors with documenting UNUSED than other conventions\nwe have.\n\nI have no objection if somebody else does a thoughtful job to\ndocument these things evenly, but if we are going to document\nUNUSED, we should explicitly document that it is our policy to\ndocument each and every one of these conventions to help new\ncontributors.\n\nThanks.\n"},{"id":"501789","messageId":"CAPig+cSBPxtv50KknJVbucAa9RGxQxP66XHpjCjECWdSGPRHjw@mail.gmail.com","threadId":"62013","inReplyTo":"20240828144814.GB4020916@coredump.intra.peff.net","subject":"Re: [PATCH 7/6] CodingGuidelines: mention -Wunused-parameter and UNUSED","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-28T19:53:53Z","receivedAt":"2024-08-28T19:54:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 28, 2024 at 10:48 AM Jeff King wrote:\n> On Wed, Aug 28, 2024 at 01:56:13AM -0400, Eric Sunshine wrote:\n> > What is the expectation regarding newcomers to the project or even\n> > people who have not been following this topic and its cousins?\n> > Documentation/CodingGuidelines recommends enabling DEVELOPER mode,\n> > which is good, but this change means that such people may now be hit\n> > with a compiler complaint which they don't necessarily know how to\n> > deal with in the legitimate case #3 (described above). Should\n> > CodingGuidelines be updated to mention \"UNUSED\" and the circumstances\n> > under which it should be used?\n>\n> Yeah, I agree some guidance is in order. How about this on top? I didn't\n> go into the decision tree of when you should remove the parameter versus\n> using it versus annotating it. I think in general that the first two are\n> pretty obvious when they are appropriate, and we just need to focus on\n> the annotating option.\n>\n> -- >8 --\n> Subject: [PATCH] CodingGuidelines: mention -Wunused-parameter and UNUSED\n>\n> Now that -Wunused-parameter is on by default for DEVELOPER=1 builds,\n> people may trigger it, blocking their build. When it's a mistake for the\n> parameter to exist, the path forward is obvious: remove it. But\n> sometimes you need to suppress the warning, and the \"UNUSED\" mechanism\n> for that is specific to our project, so people may not know about it.\n>\n> Let's put some advice in CodingGuidelines, including an example warning\n> message. That should help people who grep for the warning text after\n> seeing it from the compiler.\n\nMakes sense.\n\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> @@ -258,6 +258,13 @@ For C programs:\n> + - When using DEVELOPER=1 mode, you may see warnings from the compiler\n> + like \"error: unused parameter 'foo' [-Werror=unused-parameter]\",\n> + which indicates that a function ignores its argument. If the unused\n> + parameter can't be removed (e.g., because the function is used as a\n> + callback and has to match a certain interface), you can annotate the\n> + individual parameters with the UNUSED keyword, like \"int foo UNUSED\".\n\nPerfect. This fully addresses the question expressed by my review\ncomment. Thank you.\n"}]}