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@xxxxxxxxxxxxxxxxxxxxx/
- https://lore.kernel.org/git/20181102052239.GA19162@xxxxxxxxxxxxxxxxxxxxx/
