[PATCH] khash: silence -Wunused-function

[PATCH] khash: silence -Wunused-function

[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.

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... */
 
-- 
2.19.1

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

[Duy Nguyen]

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

>
>  /* Other convenient macros... */
>
> --
> 2.19.1
>


-- 
Duy

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

[Carlo Marcelo Arenas Belón]

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

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

[Carlo Marcelo Arenas Belón]

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(-)

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

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

[Jeff King]

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.

> 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?

> 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

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

[Carlo Arenas]

On Tue, Oct 23, 2018 at 3:00 PM Jeff King <peff@peff.net> wrote:
> 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

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

[Carlo Marcelo Arenas Belón]

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(-)

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

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

[René Scharfe]

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?

> 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é

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

[Carlo Arenas]

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