Re: [PATCH] git-compat-util.h: GCC deprecated only takes a message in GCC 4.5+
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Oct 6, 2022, 21:15 UTC
- Message-ID
- <221006.86edvkr6cc.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <Yz7HGAThrOcPdmjm@coredump.intra.peff.net>
On Thu, Oct 06 2022, Jeff King wrote:
Show 18 quoted lines
> 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.