Re: [PATCH] git-compat-util.h: GCC deprecated only takes a message in GCC 4.5+

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

On Thu, Oct 06 2022, Jeff King wrote:

> On Thu, Oct 06, 2022 at 09:29:11AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > This will cause some mild hardships, as later patches will need to
>> > #define UNUSED in other spots, as well, in order to get full coverage of
>> > the code base (I have written those annotation patches, but they're not
>> > applied upstream yet).
>> 
>> Sorry about any trouble in having to rebase those on UNUSED.
>
> That part was not too bad, and is already done.
>
> The trickiest part is that the headers get included in odd orders, and
> if the macros don't match, the compiler will complain (this has to do
> with compat/ headers which don't necessarily start by including
> git-compat-util.h).
>
> But if the definition gets much more complicated, then it's probably
> worth pulling it out rather than repeating it.

Yeah, I've dealt with that pain before in other contexts. It would be
great to have a git-compiler-compat.h with just the various
__attribute__ stuff split off from git-compat-util.h.

Maybe (compiles, but otherwise untested):

 git-compat-util.h   | 52 +-------------------------------------------------
 git-compiler-util.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 51 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index b90b64718eb..bfa44921c03 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1,18 +1,6 @@
 #ifndef GIT_COMPAT_UTIL_H
 #define GIT_COMPAT_UTIL_H
-
-#if __STDC_VERSION__ - 0 < 199901L
-/*
- * Git is in a testing period for mandatory C99 support in the compiler.  If
- * your compiler is reasonably recent, you can try to enable C99 support (or,
- * for MSVC, C11 support).  If you encounter a problem and can't enable C99
- * support with your compiler (such as with "-std=gnu99") and don't have access
- * to one with this support, such as GCC or Clang, you can remove this #if
- * directive, but please report the details of your system to
- * git@vger.kernel.org.
- */
-#error "Required C99 support is in a test phase.  Please see git-compat-util.h for more details."
-#endif
+#include "git-compiler-util.h"
 
 #ifdef USE_MSVC_CRTDBG
 /*
@@ -189,13 +177,6 @@ struct strbuf;
 #define _NETBSD_SOURCE 1
 #define _SGI_SOURCE 1
 
-#if defined(__GNUC__)
-#define UNUSED __attribute__((unused)) \
-	__attribute__((deprecated ("parameter declared as UNUSED")))
-#else
-#define UNUSED
-#endif
-
 #if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */
 # if !defined(_WIN32_WINNT)
 #  define _WIN32_WINNT 0x0600
@@ -557,37 +538,6 @@ static inline int git_has_dir_sep(const char *path)
 #endif
 #endif
 
-#if defined(__HP_cc) && (__HP_cc >= 61000)
-#define NORETURN __attribute__((noreturn))
-#define NORETURN_PTR
-#elif defined(__GNUC__) && !defined(NO_NORETURN)
-#define NORETURN __attribute__((__noreturn__))
-#define NORETURN_PTR __attribute__((__noreturn__))
-#elif defined(_MSC_VER)
-#define NORETURN __declspec(noreturn)
-#define NORETURN_PTR
-#else
-#define NORETURN
-#define NORETURN_PTR
-#ifndef __GNUC__
-#ifndef __attribute__
-#define __attribute__(x)
-#endif
-#endif
-#endif
-
-/* The sentinel attribute is valid from gcc version 4.0 */
-#if defined(__GNUC__) && (__GNUC__ >= 4)
-#define LAST_ARG_MUST_BE_NULL __attribute__((sentinel))
-/* warn_unused_result exists as of gcc 3.4.0, but be lazy and check 4.0 */
-#define RESULT_MUST_BE_USED __attribute__ ((warn_unused_result))
-#else
-#define LAST_ARG_MUST_BE_NULL
-#define RESULT_MUST_BE_USED
-#endif
-
-#define MAYBE_UNUSED __attribute__((__unused__))
-
 #include "compat/bswap.h"
 
 #include "wildmatch.h"
diff --git a/git-compiler-util.h b/git-compiler-util.h
new file mode 100644
index 00000000000..25fa0bc65d7
--- /dev/null
+++ b/git-compiler-util.h
@@ -0,0 +1,55 @@
+#ifndef GIT_COMPILER_UTIL_H
+#define GIT_COMPILER_UTIL_H
+
+#if __STDC_VERSION__ - 0 < 199901L
+/*
+ * Git is in a testing period for mandatory C99 support in the compiler.  If
+ * your compiler is reasonably recent, you can try to enable C99 support (or,
+ * for MSVC, C11 support).  If you encounter a problem and can't enable C99
+ * support with your compiler (such as with "-std=gnu99") and don't have access
+ * to one with this support, such as GCC or Clang, you can remove this #if
+ * directive, but please report the details of your system to
+ * git@vger.kernel.org.
+ */
+#error "Required C99 support is in a test phase.  Please see git-compiler-util.h for more details."
+#endif
+
+#if defined(__GNUC__)
+#define UNUSED __attribute__((unused)) \
+	__attribute__((deprecated ("parameter declared as UNUSED")))
+#else
+#define UNUSED
+#endif
+
+#endif
+
+#if defined(__HP_cc) && (__HP_cc >= 61000)
+#define NORETURN __attribute__((noreturn))
+#define NORETURN_PTR
+#elif defined(__GNUC__) && !defined(NO_NORETURN)
+#define NORETURN __attribute__((__noreturn__))
+#define NORETURN_PTR __attribute__((__noreturn__))
+#elif defined(_MSC_VER)
+#define NORETURN __declspec(noreturn)
+#define NORETURN_PTR
+#else
+#define NORETURN
+#define NORETURN_PTR
+#ifndef __GNUC__
+#ifndef __attribute__
+#define __attribute__(x)
+#endif
+#endif
+#endif
+
+/* The sentinel attribute is valid from gcc version 4.0 */
+#if defined(__GNUC__) && (__GNUC__ >= 4)
+#define LAST_ARG_MUST_BE_NULL __attribute__((sentinel))
+/* warn_unused_result exists as of gcc 3.4.0, but be lazy and check 4.0 */
+#define RESULT_MUST_BE_USED __attribute__ ((warn_unused_result))
+#else
+#define LAST_ARG_MUST_BE_NULL
+#define RESULT_MUST_BE_USED
+#endif
+
+#define MAYBE_UNUSED __attribute__((__unused__))

>> If you're taking requests it would be really useful to prioritize
>> changes to shared headers and the like, e.g. DEVOPTS=extra-all on pretty
>> much any file will start with:
>> 	
>> 	git-compat-util.h: In function ‘precompose_argv_prefix’:
>> 	git-compat-util.h:313:54: error: unused parameter ‘argc’ [-Werror=unused-parameter]
>> 	  313 | static inline const char *precompose_argv_prefix(int argc, const char **argv, const char *prefix)
>> 	      |                                                  ~~~~^~~~
>> 	git-compat-util.h:313:73: error: unused parameter ‘argv’ [-Werror=unused-parameter]
>> 	  313 | static inline const char *precompose_argv_prefix(int argc, const char **argv, const char *prefix)
>> 	      |                                                            ~~~~~~~~~~~~~^~~~
>> 	git-compat-util.h: In function ‘git_has_dos_drive_prefix’:
>> 	git-compat-util.h:423:56: error: unused parameter ‘path’ [-Werror=unused-parameter]
>> 	  423 | static inline int git_has_dos_drive_prefix(const char *path)
>> 	      |                                            ~~~~~~~~~~~~^~~~
>> 	git-compat-util.h: In function ‘git_skip_dos_drive_prefix’:
>> 	git-compat-util.h:431:52: error: unused parameter ‘path’ [-Werror=unused-parameter]
>> 	  431 | static inline int git_skip_dos_drive_prefix(char **path)
>
> Yeah, those are near the top of my list. I have a group classified as
> "trivial": functions which are compat placeholders and have no body.
> I'll be mostly offline for about a week, but I hope to send another
> round of unused-mark patches when I get back. (Of course it is not
> really useful until _all_ of the patches are there anyway).

I was ad-hoc testing this earlier and just dug this back out of my
stash, maybe going in something like this direction is useful:
	
	diff --git a/config.mak.dev b/config.mak.dev
	index 4fa19d361b7..60bc8c406cf 100644
	--- a/config.mak.dev
	+++ b/config.mak.dev
	@@ -54,6 +54,47 @@ DEVELOPER_CFLAGS += -Wno-empty-body
	 DEVELOPER_CFLAGS += -Wno-missing-field-initializers
	 DEVELOPER_CFLAGS += -Wno-sign-compare
	 DEVELOPER_CFLAGS += -Wno-unused-parameter
	+
	+define use-unused-parameter
	+$(1): DEVELOPER_CFLAGS += -Wunused-parameter
	+
	+endef
	+
	+TEST_BUILTINS_NO_UNUSED =
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-ctype.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-date.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-drop-caches.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-dump-cache-tree.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-dump-fsmonitor.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-dump-split-index.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-dump-untracked-cache.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-example-decorate.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-fsmonitor-client.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-index-version.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-match-trees.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-mergesort.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-oid-array.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-oidmap.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-oidtree.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-oidtree.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-online-cpus.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-parse-options.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-path-utils.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-prio-queue.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-read-graph.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-ref-store.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-run-command.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-scrap-cache-tree.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-sigchain.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-simple-ipc.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-strcmp-offset.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-submodule-config.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-trace2.o
	+TEST_BUILTINS_OBJS_NO_UNUSED += test-xml-encode.o
	+
	+TEST_BUILTINS_OBJS_CHECK_UNUSED = $(filter-out $(TEST_BUILTINS_OBJS_NO_UNUSED),$(TEST_BUILTINS_OBJS))
	+
	+$(eval $(foreach obj,$(TEST_BUILTINS_OBJS_CHECK_UNUSED:%=t/helper/%), $(call use-unused-parameter,$(obj))))
	 endif
	 endif
	 
It's probably too painful to maintain that on a per-file basis, but if
you can get to a point where e.g. t/helper/ is -Wunused-parameter clean
we can just append -Wunused-parameter to DEVELOPER_CFLAGS for those
paths.

That'll ensure that we don't have "regressions" in newly added
parameters for files we've already cleared.

Maybe not worth it, I don't know if we'd be re-adding these at a
sufficient rate to make it worth it, probably you'll send all these in
and we'll find there's maybe 1-5 easily tackled regressions before we
remove that "DEVELOPER_CFLAGS += -Wno-unused-parameter" line.