Re: [PATCH] config.mak.dev: enable -Wunused-function

From: Duy Nguyen <pclouds@gmail.com>
Date: Oct 18, 2018, 15:48 UTC
Message-ID: <CACsJy8BOL+QOb3Vdh8RKbra-DDbJVyznTbCJgjK5h8L2y0H7yg@mail.gmail.com>
In-Reply-To: <20181018070522.GA29499@sigill.intra.peff.net>

On Thu, Oct 18, 2018 at 9:05 AM Jeff King <peff@peff.net> wrote:

Show 24 quoted lines

>
> We explicitly omitted -Wunused-function when we added
> -Wextra, but there is no need: the current code compiles
> cleanly with it. And it's worth having, since it can let you
> know when there are cascading effects from a cleanup (e.g.,
> deleting one function lets you delete its static helpers).
>
> There are cases where we may need an unused function to
> exist, but we can handle these easily:
>
>   - macro-generated code like commit-slab; there we have the
>     MAYBE_UNUSED annotation to silence the compiler
>
>   - conditional compilation, where we may or may not need a
>     static helper. These generally fall into one of two
>     categories:
>
>       - the call should not be conditional, but rather the
>         function body itself should be (and may just be a
>         no-op on one side of the #if). That keeps the
>         conditional pollution out of the main code.
>
>       - call-chains of static helpers should all be in the
>         same #if block, so they are all-or-nothing

Grouping is not always desired because it could break better function layout. Have a look at read-cache.c where _ieot_extension functions are #if'd because the only call sites are from pthread code (#if'd far away).

In this particular case though I think we should be able to avoid so much #if if we make a wrapper for pthread api that would return an error or something when pthread is not available. But similar situation may happen elsewhere too.

Having said that, if people do consider MAYBE_UNUSED before #if'ing everywhere (and opening up more conditional build problems in future), I think this change is fine.

-- 
Duy