| Message ID | 20181018070522.GA29499@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config.mak.dev: enable -Wunused-function | expand |
On Thu, Oct 18, 2018 at 03:05:22AM -0400, Jeff King wrote: > diff --git a/config.mak.dev b/config.mak.dev > index 92d268137f..bbeeff44fe 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -34,7 +34,6 @@ ifeq ($(filter extra-all,$(DEVOPTS)),) > CFLAGS += -Wno-empty-body > CFLAGS += -Wno-missing-field-initializers > CFLAGS += -Wno-sign-compare > -CFLAGS += -Wno-unused-function > CFLAGS += -Wno-unused-parameter By the way, I wondered how close we were to being able to use -Wunused-parameter. The answer is "not very". However, I've been digging into the results, and it does find a number of bugs. I'm 168 (rough) patches deep right now, and I have it compiling cleanly. Most of those are just annotations, but I'll start posting fixes as I organize and clean them up. -Peff
On Thu, Oct 18, 2018 at 9:05 AM Jeff King <peff@peff.net> wrote: > > 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.
On 18/10/2018 08:05, Jeff King wrote: > 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 > > And if there's some case that doesn't cover, we still > have MAYBE_UNUSED as a fallback. > > Signed-off-by: Jeff King <peff@peff.net> > --- > config.mak.dev | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/config.mak.dev b/config.mak.dev > index 92d268137f..bbeeff44fe 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -34,7 +34,6 @@ ifeq ($(filter extra-all,$(DEVOPTS)),) > CFLAGS += -Wno-empty-body > CFLAGS += -Wno-missing-field-initializers > CFLAGS += -Wno-sign-compare > -CFLAGS += -Wno-unused-function > CFLAGS += -Wno-unused-parameter > endif > endif > Heh, as luck would have it, tonight I had an -Wunused-function warning on cygwin, but not Linux, when building the 'pu' branch. [On linux, I am using DEVELOPER=1 in my config.mak etc., whereas on cygwin I am still using an explicit (but very similar) list of -W args.] I haven't looked too deeply, but this seems to be caused by Junio's commit 42c89ea70a ("SQUASH??? - convert the other user of string-list as db", 2018-10-17) which removes a call to the add_existing() function - the subject of the warning. [BTW there is another 'static add_existing()' in builtin/show_ref.c] ATB, Ramsay Jones
On Thu, Oct 18, 2018 at 05:48:16PM +0200, Duy Nguyen wrote: > > - 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). True, though as long as they are triggered by the same set of #if conditions, that is fine. Putting them in the same block is just an easy way to make sure that is the case. ;) > 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. Yeah, I think that is generally the preferred method anyway, just because of readability and simplicity. > 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. I'd like to use it as a last resort, certainly. Mostly the fact that we compile cleanly _now_ makes me think that it probably won't be that hard to keep it going. I think the biggest potential problem with this is going to be obscure configurations where some functions are used or not used. So somebody silencing a compiler warning may inadvertently break another case if they're not careful. But that's already a problem to some degree (and part of why we try to push the conditionality out to the whole-function level). -Peff
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > I haven't looked too deeply, but this seems to be caused by > Junio's commit 42c89ea70a ("SQUASH??? - convert the other user of > string-list as db", 2018-10-17) which removes a call to the > add_existing() function - the subject of the warning. That is very understandable and it is immediately obvious to me that the unused-function warning is being useful there ;-)
diff --git a/config.mak.dev b/config.mak.dev index 92d268137f..bbeeff44fe 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -34,7 +34,6 @@ ifeq ($(filter extra-all,$(DEVOPTS)),) CFLAGS += -Wno-empty-body CFLAGS += -Wno-missing-field-initializers CFLAGS += -Wno-sign-compare -CFLAGS += -Wno-unused-function CFLAGS += -Wno-unused-parameter endif endif
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 And if there's some case that doesn't cover, we still have MAYBE_UNUSED as a fallback. Signed-off-by: Jeff King <peff@peff.net> --- config.mak.dev | 1 - 1 file changed, 1 deletion(-)