[PATCH 0/11] annotating unused function parameters

I've been carrying a bunch of patches (for almost 4 years now!) that get the code base compiling cleanly with -Wunused-parameter. This is a useful warning in my opinion; it found real bugs[1] when applied to the whole code base. So it would be nice to be able to turn it on all the time and get the same protection going forward.

Unfortunately, there's a catch: some functions really do need to have unused parameters, because they're conforming to a specific interface. Some reasons might be:

  - they're passed as callback function pointers

  - they're virtual functions called as pointers via a struct

  - they're compat functions that implement a well-known interface (this
    is especially true for "trivial" wrappers that are noops, or return
    ENOSYS, etc)

So we need some way to annotate the acceptable cases. This series introduces a macro to do so. That's in the first patch. The other ten apply it in various situations, with the end goal being that we could flip the warning on for DEVELOPER=1 builds. This series doesn't get us all the way there! I have ~100 more patches adding similar annotations. My goal here is to give a taste of the direction and make sure it's where we want to go.

I've grouped related annotations into a single patch. They should be fairly easy to review. The annotation macro is written in such a way that the compiler will complain if it's wrong. So really what you care about in each is:

  - is this a case where we really want to be annotating and not
    removing unused parameters (or using them)? Each commit should
    explain this, and it's usually obvious (e.g., a specific type of
    callback)

  - does each hunk in the patch match the group

  - did I introduce any formatting problems along the way :)

  - (optional) are there other cases that should be in the group. This
    is probably not worth your time to go digging for. The other ~100
    patches aren't carefully organized and written up yet, so it's quite
    possible that I've mistakenly left a related case there that _could_
    be folded in. But those shake out much easier as the number of cases
    is reduced, and you can ask -Wunused-parameter to find them for you.

And of course the most important question is: do we like this direction overall. This mass-annotation is a one-time pain. Going forward, the only work would be requiring people to annotate new functions they add (which again, is mostly going to be callbacks). IMHO it's worth it. In addition to possibly finding errors, I think the annotations serve as an extra clue for people reading the code about what the author intended.

So without further ado, the patches are:

  [01/11]: git-compat-util: add UNUSED macro
  [02/11]: refs: mark unused each_ref_fn parameters
  [03/11]: refs: mark unused reflog callback parameters
  [04/11]: refs: mark unused virtual method parameters
  [05/11]: transport: mark bundle transport_options as unused
  [06/11]: streaming: mark unused virtual method parameters
  [07/11]: config: mark unused callback parameters
  [08/11]: hashmap: mark unused callback parameters
  [09/11]: mark unused read_tree_recursive() callback parameters
  [10/11]: run-command: mark unused async callback parameters
  [11/11]: is_path_owned_by_current_uid(): mark "report" parameter as unused

 add-interactive.c           |  2 +-
 archive-tar.c               |  5 +++--
 archive-zip.c               |  5 +++--
 archive.c                   |  3 ++-
 attr.c                      |  4 ++--
 bisect.c                    |  7 ++++---
 bloom.c                     |  4 ++--
 builtin/am.c                |  2 +-
 builtin/bisect--helper.c    | 12 +++++++-----
 builtin/checkout.c          |  4 ++--
 builtin/commit-graph.c      |  2 +-
 builtin/config.c            |  8 +++++---
 builtin/describe.c          |  5 +++--
 builtin/difftool.c          | 10 +++++-----
 builtin/fast-export.c       |  2 +-
 builtin/fast-import.c       |  2 +-
 builtin/fetch.c             |  9 +++++----
 builtin/fsck.c              | 12 +++++++-----
 builtin/gc.c                |  5 +++--
 builtin/log.c               |  7 ++++---
 builtin/ls-tree.c           | 13 ++++++++-----
 builtin/multi-pack-index.c  |  2 +-
 builtin/name-rev.c          |  3 ++-
 builtin/pack-objects.c      | 12 +++++++-----
 builtin/receive-pack.c      |  4 ++--
 builtin/reflog.c            |  3 ++-
 builtin/remote.c            | 14 +++++++++-----
 builtin/repack.c            |  4 ++--
 builtin/rev-parse.c         |  6 ++++--
 builtin/show-branch.c       |  6 +++---
 builtin/show-ref.c          |  7 ++++---
 builtin/stash.c             |  9 ++++++---
 builtin/submodule--helper.c |  5 +++--
 color.c                     |  2 +-
 commit-graph.c              |  4 ++--
 commit.c                    |  5 +++--
 compat/terminal.c           |  2 +-
 config.c                    |  7 ++++---
 convert.c                   |  4 ++--
 delta-islands.c             |  4 ++--
 diff.c                      |  5 +++--
 dir.c                       |  4 ++--
 environment.c               |  4 ++--
 fetch-pack.c                | 14 +++++++++-----
 git-compat-util.h           | 13 +++++++++++--
 gpg-interface.c             |  2 +-
 hashmap.c                   | 10 +++++-----
 help.c                      |  5 +++--
 http-backend.c              |  2 +-
 ident.c                     |  2 +-
 ll-merge.c                  |  3 ++-
 log-tree.c                  |  3 ++-
 ls-refs.c                   |  3 ++-
 merge-recursive.c           | 12 ++++++------
 name-hash.c                 |  4 ++--
 negotiator/default.c        |  3 ++-
 negotiator/skipping.c       |  3 ++-
 notes.c                     |  5 +++--
 object-name.c               | 10 +++++++---
 object-store.h              |  2 +-
 oidmap.c                    |  2 +-
 packfile.c                  |  2 +-
 pager.c                     |  3 ++-
 patch-ids.c                 |  2 +-
 pretty.c                    |  3 ++-
 range-diff.c                |  6 ++++--
 ref-filter.c                |  2 +-
 reflog.c                    | 16 ++++++++++------
 refs.c                      | 23 ++++++++++++++---------
 refs/files-backend.c        | 14 ++++++++------
 refs/iterator.c             |  6 +++---
 refs/packed-backend.c       | 14 ++++++++------
 remote.c                    | 22 +++++++++++++---------
 replace-object.c            |  3 ++-
 revision.c                  | 18 +++++++++++-------
 send-pack.c                 |  2 +-
 sequencer.c                 |  5 +++--
 server-info.c               |  3 ++-
 shallow.c                   | 12 ++++++++----
 streaming.c                 |  6 +++---
 strmap.c                    |  4 ++--
 sub-process.c               |  4 ++--
 submodule-config.c          |  8 ++++----
 submodule.c                 | 13 ++++++++-----
 t/helper/test-config.c      |  2 +-
 t/helper/test-ref-store.c   |  4 ++--
 t/helper/test-userdiff.c    |  2 +-
 trailer.c                   |  6 ++++--
 transport.c                 |  2 +-
 upload-pack.c               |  7 ++++---
 walker.c                    |  6 ++++--
 wt-status.c                 | 14 +++++++++-----
 92 files changed, 337 insertions(+), 234 deletions(-)

-Peff

[1] Here are some examples of bugs found by -Wunused-parameter:

     - https://lore.kernel.org/git/20181102063156.GA30252@sigill.intra.peff.net/
     - https://lore.kernel.org/git/20181102052239.GA19162@sigill.intra.peff.net/