threads / patch / 49650

patchkhash: silence -Wunused-function

Subject: [PATCH] khash: silence -Wunused-function

## tl;dr

9 messages between Oct 23, 2018 and Oct 23, 2018. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Carlo Marcelo Arenas Belón· Oct 23, 2018, 11:34 UTC · lore

after 36da893114 ("config.mak.dev: enable -Wunused-function", 2018-10-18) macro generated code should use a similar solution than commit-slab to silence the compiler.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 khash.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to khash.h +1 −1
diff --git a/khash.h b/khash.h
index d10caa0c35..39c2833877 100644
--- a/khash.h
+++ b/khash.h
@@ -234,7 +234,7 @@ static const double __ac_HASH_UPPER = 0.77;
 	__KHASH_IMPL(name, SCOPE, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal)
 
 #define KHASH_INIT(name, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal) \
-	KHASH_INIT2(name, static inline, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal)
+	KHASH_INIT2(name, __attribute__((__unused__)) static inline, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal)
 
 /* Other convenient macros... */
 

-- 
2.19.1
Duy Nguyen· Oct 23, 2018, 15:44 UTC · re: Carlo Marcelo Arenas Belón · lore

Re: [PATCH] khash: silence -Wunused-function

On Tue, Oct 23, 2018 at 1:42 PM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

Show 20 quoted lines
>
> after 36da893114 ("config.mak.dev: enable -Wunused-function", 2018-10-18)
> macro generated code should use a similar solution than commit-slab to
> silence the compiler.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  khash.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/khash.h b/khash.h
> index d10caa0c35..39c2833877 100644
> --- a/khash.h
> +++ b/khash.h
> @@ -234,7 +234,7 @@ static const double __ac_HASH_UPPER = 0.77;
>         __KHASH_IMPL(name, SCOPE, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal)
>
>  #define KHASH_INIT(name, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal) \
> -       KHASH_INIT2(name, static inline, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal)
> +       KHASH_INIT2(name, __attribute__((__unused__)) static inline, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal)

Maybe move MAYBE_UNUSED from commit-slab-impl.h to git-compat-util.h and use it here so we have an easier time if we have to use a different way to mark unused functions on some exotic platform.

Show 6 quoted lines
>
>  /* Other convenient macros... */
>
> --
> 2.19.1
>
-- 
Duy
Carlo Marcelo Arenas Belón· Oct 23, 2018, 21:50 UTC · re: Duy Nguyen · lore

[PATCH v2 0/2] delta-islands: avoid unused function messages

the macro generated code from delta-islands (using khash) triggers some unused function warnings in macOS, OpenBSD and some linux with a newer version of clang

Carlo Marcelo Arenas Belón (2):
  commit-slabs: move MAYBE_UNUSED out
  khash: silence -Wunused-function for delta-islands
 commit-slab-impl.h | 4 ++--
 git-compat-util.h  | 2 ++
 khash.h            | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)
-- 
2.19.1
Carlo Marcelo Arenas Belón· Oct 23, 2018, 21:50 UTC · re: Carlo Marcelo Arenas Belón · lore

[PATCH v2 1/2] commit-slabs: move MAYBE_UNUSED out

after 36da893114 ("config.mak.dev: enable -Wunused-function", 2018-10-18) it is expected to be used to prevent -Wunused-function warnings for code that was macro generated

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 commit-slab-impl.h | 4 ++--
 git-compat-util.h  | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)
Show changes to 2 files +4 −2

commit-slab-impl.h, git-compat-util.h

diff --git a/commit-slab-impl.h b/commit-slab-impl.h
index ac1e6d409a..5c0eb91a5d 100644
--- a/commit-slab-impl.h
+++ b/commit-slab-impl.h
@@ -1,10 +1,10 @@
 #ifndef COMMIT_SLAB_IMPL_H
 #define COMMIT_SLAB_IMPL_H
 
-#define MAYBE_UNUSED __attribute__((__unused__))
+#include "git-compat-util.h"
 
 #define implement_static_commit_slab(slabname, elemtype) \
-	implement_commit_slab(slabname, elemtype, static MAYBE_UNUSED)
+	implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static)
 
 #define implement_shared_commit_slab(slabname, elemtype) \
 	implement_commit_slab(slabname, elemtype, )
diff --git a/git-compat-util.h b/git-compat-util.h
index 9a64998b24..e4d3967a23 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -408,6 +408,8 @@ static inline char *git_find_last_dir_sep(const char *path)
 #define LAST_ARG_MUST_BE_NULL
 #endif
 
+#define MAYBE_UNUSED __attribute__((__unused__))
+
 #include "compat/bswap.h"
 
 #include "wildmatch.h"

-- 
2.19.1
Jeff King· Oct 23, 2018, 22:00 UTC · re: Carlo Marcelo Arenas Belón · lore

Re: [PATCH v2 1/2] commit-slabs: move MAYBE_UNUSED out

On Tue, Oct 23, 2018 at 02:50:19PM -0700, Carlo Marcelo Arenas Belón wrote:
> after 36da893114 ("config.mak.dev: enable -Wunused-function", 2018-10-18)
> it is expected to be used to prevent -Wunused-function warnings for code
> that was macro generated
Makes sense.
Show 10 quoted lines
> diff --git a/commit-slab-impl.h b/commit-slab-impl.h
> index ac1e6d409a..5c0eb91a5d 100644
> --- a/commit-slab-impl.h
> +++ b/commit-slab-impl.h
> @@ -1,10 +1,10 @@
>  #ifndef COMMIT_SLAB_IMPL_H
>  #define COMMIT_SLAB_IMPL_H
>  
> -#define MAYBE_UNUSED __attribute__((__unused__))
> +#include "git-compat-util.h"

We shouldn't need to include git-compat-util.h, as the C files would already do so, via this rule in CodingGuidelines:

   - The first #include in C files, except in platform specific compat/
   implementations, must be either "git-compat-util.h", "cache.h" or
   "builtin.h".  You do not have to include more than one of these.
(and if there is a case that does not, we should fix that).
>  #define implement_static_commit_slab(slabname, elemtype) \
> -	implement_commit_slab(slabname, elemtype, static MAYBE_UNUSED)
> +	implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static)
Is this hunk necessary?
Show 11 quoted lines
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 9a64998b24..e4d3967a23 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -408,6 +408,8 @@ static inline char *git_find_last_dir_sep(const char *path)
>  #define LAST_ARG_MUST_BE_NULL
>  #endif
>  
> +#define MAYBE_UNUSED __attribute__((__unused__))
> +
>  #include "compat/bswap.h"
Looks good.
-Peff
Carlo Arenas· Oct 23, 2018, 23:02 UTC · re: Jeff King · lore

Re: [PATCH v2 1/2] commit-slabs: move MAYBE_UNUSED out

On Tue, Oct 23, 2018 at 3:00 PM Jeff King <peff@peff.net> wrote:
Show 6 quoted lines
> On Tue, Oct 23, 2018 at 02:50:19PM -0700, Carlo Marcelo Arenas Belón wrote:
> >  #define implement_static_commit_slab(slabname, elemtype) \
> > -     implement_commit_slab(slabname, elemtype, static MAYBE_UNUSED)
> > +     implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static)
>
> Is this hunk necessary?

it works eitherway but the proposed syntax is IMHO better aligned (when used in a function definition) as it matches better the syntax from C++ attribute: maybe_unused (since C++17) [1]

__attribute__(unused) (the GCC extension) is meant to be applied to function declarations, but we are not generating those.

Carlo
[1] https://en.cppreference.com/w/cpp/language/attributes/maybe_unused
Carlo Marcelo Arenas Belón· Oct 23, 2018, 21:50 UTC · re: Carlo Marcelo Arenas Belón · lore

[PATCH v2 2/2] khash: silence -Wunused-function for delta-islands

showing the following when compiled with latest clang (OpenBSD, Fedors and macOS):

delta-islands.c:23:1: warning: unused function 'kh_destroy_str'
      [-Wunused-function]
delta-islands.c:23:1: warning: unused function 'kh_clear_str'
      [-Wunused-function]
delta-islands.c:23:1: warning: unused function 'kh_del_str' [-Wunused-function]
Reported-by: René Scharfe <l.s.r@web.de>
Suggested-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 khash.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to khash.h +1 −1
diff --git a/khash.h b/khash.h
index c0da40daa7..fb71c92547 100644
--- a/khash.h
+++ b/khash.h
@@ -226,7 +226,7 @@ static const double __ac_HASH_UPPER = 0.77;
 	__KHASH_IMPL(name, SCOPE, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal)
 
 #define KHASH_INIT(name, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal) \
-	KHASH_INIT2(name, static inline, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal)
+	KHASH_INIT2(name, MAYBE_UNUSED static inline, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal)
 
 /* Other convenient macros... */
 

-- 
2.19.1
René Scharfe· Oct 23, 2018, 16:19 UTC · re: Carlo Marcelo Arenas Belón · lore

Re: [PATCH] khash: silence -Wunused-function

Am 23.10.2018 um 13:34 schrieb Carlo Marcelo Arenas Belón:
> after 36da893114 ("config.mak.dev: enable -Wunused-function", 2018-10-18)
> macro generated code should use a similar solution than commit-slab to
> silence the compiler.

With Clang 6 and GCC 8 on Debian I don't get any warnings on master or 36da893114.

With Clang 6 on OpenBSD I get warnings about the unused khash functions in delta-islands.c on master, though.

What do you get, or in other words: why did you send this patch?
Show 18 quoted lines
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  khash.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/khash.h b/khash.h
> index d10caa0c35..39c2833877 100644
> --- a/khash.h
> +++ b/khash.h
> @@ -234,7 +234,7 @@ static const double __ac_HASH_UPPER = 0.77;
>  	__KHASH_IMPL(name, SCOPE, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal)
>  
>  #define KHASH_INIT(name, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal) \
> -	KHASH_INIT2(name, static inline, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal)
> +	KHASH_INIT2(name, __attribute__((__unused__)) static inline, khkey_t, khval_t, kh_is_map, __hash_func, __hash_equal)
>  
>  /* Other convenient macros... */
>

This fixes the warning on OpenBSD for me. So does moving the KHASH_INIT line from delta-islands.c to a header file (e.g. khash.h or delta-islands.h).

René
Carlo Arenas· Oct 23, 2018, 16:52 UTC · re: René Scharfe · lore

Re: [PATCH] khash: silence -Wunused-function

On Tue, Oct 23, 2018 at 9:19 AM René Scharfe <l.s.r@web.de> wrote:
> With Clang 6 and GCC 8 on Debian I don't get any warnings on master or
> 36da893114.

I see it with Clang 7 on Fedora (at least Rawhide but suspect also to affect the next release, now in beta: 29)

> With Clang 6 on OpenBSD I get warnings about the unused khash functions
> in delta-islands.c on master, though.

Apple LLVM 9 and 10 will also trigger similar warnings to what you see in OpenBSD and as Junio mentioned recently in a different thread about semantic patches, this currently affects the CI for "Apple":

  https://travis-ci.org/git/git/builds/444969342
Carlo

← back to recent threads