| Message ID | Yv9gxqH6nK2KYnNj@coredump.intra.peff.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | annotating unused function parameters | expand |
On Fri, Aug 19 2022, Jeff King wrote: > 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. > [...] > 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. I've known you've had this out-of-tree for a while, and really like that it's on the path to getting integrated. But I have a hang-up about it, which is that I though __attribute__ (unused) didn't work like *that*. What it means (and maybe only I find this counter-intuitive) is "trust me, this is unused, but don't check!", furthermore it causes the compiler to completely ignore the variable for the purposes of *all* warnings, not just the unused one. I may still be missing something, but I wonder if this squashed in wouldn't be much better: diff --git a/git-compat-util.h b/git-compat-util.h index a9690126bb0..e02e2fc3f6d 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -190,9 +190,9 @@ struct strbuf; #define _SGI_SOURCE 1 #if defined(__GNUC__) -#define UNUSED(var) UNUSED_##var __attribute__((unused)) +#define UNUSED(var) var __attribute__((deprecated ("not 'deprecated', but expected not to be used!"))) #else -#define UNUSED(var) UNUSED_##var +#define UNUSED(var) var #endif #if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */ I.e. it's a bit counter-intuitive to mark these as "deprecated", but you can add a custom message (with both GCC and clang). Improvements to the message welcome. Now as in this series we stay silent if the variable is not used, but we *don't* stay silent if an UNUSED(var) is actually used, that'll now be an error: xdiff/xdiffi.c: In function ‘xdl_call_hunk_func’: xdiff/xdiffi.c:981:9: error: ‘xe’ is deprecated: not 'deprecated', but expected not to be used! [-Werror=deprecated-declarations] 981 | fprintf(stderr, "%p", (void*)xe); | ^~~~~~~ This also means that you don't need to rename the variable just to avoid "accidental" use, which also has the benefit of not tripping up the variable typo detection.
On 8/19/2022 9:58 AM, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Aug 19 2022, Jeff King wrote: > >> 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. >> [...] >> 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. > > I've known you've had this out-of-tree for a while, and really like that > it's on the path to getting integrated. > > But I have a hang-up about it, which is that I though __attribute__ > (unused) didn't work like *that*. > > What it means (and maybe only I find this counter-intuitive) is "trust > me, this is unused, but don't check!", furthermore it causes the > compiler to completely ignore the variable for the purposes of *all* > warnings, not just the unused one. That's not the reason for the attribute at all. It's supposed to say "I know this is unused, but I still need it to be in the parameter list for other reasons. Don't create a warning for this case." Interpreting it the way you are means "don't do the analysis. Just throw a warning." which doesn't make any sense. > I may still be missing something, but I wonder if this squashed in > wouldn't be much better: > > diff --git a/git-compat-util.h b/git-compat-util.h > index a9690126bb0..e02e2fc3f6d 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -190,9 +190,9 @@ struct strbuf; > #define _SGI_SOURCE 1 > > #if defined(__GNUC__) > -#define UNUSED(var) UNUSED_##var __attribute__((unused)) > +#define UNUSED(var) var __attribute__((deprecated ("not 'deprecated', but expected not to be used!"))) > #else > -#define UNUSED(var) UNUSED_##var > +#define UNUSED(var) var > #endif Does the deprecated attribute imply unused? Or at the very least, does it avoid the -Wunused-parameter warnings? It might be helpful to _also_ have a deprecated annotation so we know to remove the UNUSED macro if a parameter starts being used again. The existing macro changes the variable name so we would get compiler errors if we started using it, but we could have a better message indicating exactly why things are not working. So in that sense, you are onto something. Should we use both attributes? At the very least, the warning message you recommend in the 'deprecated' attribute could be more direct about what we expect. #if defined(__GNUC__) -#define UNUSED(var) UNUSED_##var __attribute__((unused)) +#define UNUSED(var) var __attribute__((unused)) \ __attribute__((deprecated ("remove UNUSED macro before using"))) #else -#define UNUSED(var) UNUSED_##var +#define UNUSED(var) var #endif #if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */ Thanks, -Stolee
On Fri, Aug 19 2022, Derrick Stolee wrote: > On 8/19/2022 9:58 AM, Ævar Arnfjörð Bjarmason wrote: >> >> On Fri, Aug 19 2022, Jeff King wrote: >> >>> 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. >>> [...] >>> 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. >> >> I've known you've had this out-of-tree for a while, and really like that >> it's on the path to getting integrated. >> >> But I have a hang-up about it, which is that I though __attribute__ >> (unused) didn't work like *that*. >> >> What it means (and maybe only I find this counter-intuitive) is "trust >> me, this is unused, but don't check!", furthermore it causes the >> compiler to completely ignore the variable for the purposes of *all* >> warnings, not just the unused one. > > That's not the reason for the attribute at all. It's supposed to say "I > know this is unused, but I still need it to be in the parameter list for > other reasons. Don't create a warning for this case." > > Interpreting it the way you are means "don't do the analysis. Just throw a > warning." which doesn't make any sense. > >> I may still be missing something, but I wonder if this squashed in >> wouldn't be much better: >> >> diff --git a/git-compat-util.h b/git-compat-util.h >> index a9690126bb0..e02e2fc3f6d 100644 >> --- a/git-compat-util.h >> +++ b/git-compat-util.h >> @@ -190,9 +190,9 @@ struct strbuf; >> #define _SGI_SOURCE 1 >> >> #if defined(__GNUC__) >> -#define UNUSED(var) UNUSED_##var __attribute__((unused)) >> +#define UNUSED(var) var __attribute__((deprecated ("not 'deprecated', but expected not to be used!"))) >> #else >> -#define UNUSED(var) UNUSED_##var >> +#define UNUSED(var) var >> #endif > > Does the deprecated attribute imply unused? Or at the very least, does it > avoid the -Wunused-parameter warnings? > > It might be helpful to _also_ have a deprecated annotation so we know to > remove the UNUSED macro if a parameter starts being used again. The > existing macro changes the variable name so we would get compiler errors > if we started using it, but we could have a better message indicating > exactly why things are not working. > > So in that sense, you are onto something. Should we use both attributes? > > At the very least, the warning message you recommend in the 'deprecated' > attribute could be more direct about what we expect. > > #if defined(__GNUC__) > -#define UNUSED(var) UNUSED_##var __attribute__((unused)) > +#define UNUSED(var) var __attribute__((unused)) \ > __attribute__((deprecated ("remove UNUSED macro before using"))) > #else > -#define UNUSED(var) UNUSED_##var > +#define UNUSED(var) var > #endif > > #if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */ Yes, I spoke too soon, sorry. We still need ((unused)). FWIW the below on top of master and doing: make bisect.o CFLAGS=-Wunused-parameter Does the right thing for me, and then warns if you uncomment that "//" commented fprintf(). The reason I was confused is that -Wunused-parameter is *not* needed with ((deprecated)) to error if the variable is used, but it *is* needed to hide it from -Wunused-parameter if you're trying to find the next thing to mark as "UNUSED" (or to refactor away). I think the below version of it also shows that you don't need to pass the variable name to the macro. If the only reason for that was to avoid accidental use it seems like the ((deprecated)) attribute should cover it. I'd prefer it if we could avoid the funny syntax, it's highlighted a bit weirdly in my editor, and I suspect I'm not the only one, but mainly having the extra compiler-enforced sanity checking of checking if it's accidentally used, without renaming the variable, which seems like a bit of a hack. diff --git a/bisect.c b/bisect.c index 38b3891f3a6..fd581b85a72 100644 --- a/bisect.c +++ b/bisect.c @@ -441,7 +441,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches, } static int register_ref(const char *refname, const struct object_id *oid, - int flags, void *cb_data) + int flags UNUSED, void *cb_data UNUSED) { struct strbuf good_prefix = STRBUF_INIT; strbuf_addstr(&good_prefix, term_good); @@ -1160,8 +1160,9 @@ int estimate_bisect_steps(int all) return (e < 3 * x) ? n : n - 1; } -static int mark_for_removal(const char *refname, const struct object_id *oid, - int flag, void *cb_data) +static int mark_for_removal(const char *refname, + const struct object_id *oid UNUSED, + int flag UNUSED, void *cb_data) { struct string_list *refs = cb_data; char *ref = xstrfmt("refs/bisect%s", refname); diff --git a/git-compat-util.h b/git-compat-util.h index 36a25ae252f..7f7395fc9f7 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -189,6 +189,12 @@ struct strbuf; #define _NETBSD_SOURCE 1 #define _SGI_SOURCE 1 +#if defined(__GNUC__) +#define UNUSED __attribute__((unused)) __attribute__((deprecated ("marked with UNUSED"))) +#else +#define UNUSED +#endif + #if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */ # if !defined(_WIN32_WINNT) # define _WIN32_WINNT 0x0600 @@ -302,7 +308,8 @@ typedef unsigned long uintptr_t; #ifdef PRECOMPOSE_UNICODE #include "compat/precompose_utf8.h" #else -static inline const char *precompose_argv_prefix(int argc, const char **argv, const char *prefix) +static inline const char *precompose_argv_prefix(int argc UNUSED, const char **argv UNUSED, + const char *prefix) { return prefix; } @@ -397,7 +404,8 @@ typedef uintmax_t timestamp_t; #endif #ifndef platform_core_config -static inline int noop_core_config(const char *var, const char *value, void *cb) +static inline int noop_core_config(const char *var UNUSED, const char *value UNUSED, + void *cb UNUSED) { return 0; } @@ -410,7 +418,7 @@ int lstat_cache_aware_rmdir(const char *path); #endif #ifndef has_dos_drive_prefix -static inline int git_has_dos_drive_prefix(const char *path) +static inline int git_has_dos_drive_prefix(const char *path UNUSED) { return 0; } @@ -418,7 +426,7 @@ static inline int git_has_dos_drive_prefix(const char *path) #endif #ifndef skip_dos_drive_prefix -static inline int git_skip_dos_drive_prefix(char **path) +static inline int git_skip_dos_drive_prefix(char **path UNUSED) { return 0; } @@ -490,11 +498,13 @@ static inline void extract_id_from_env(const char *env, uid_t *id) } } -static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report) +static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report UNUSED) { struct stat st; uid_t euid; + //fprintf(stderr, "%p", (void*)report); + if (lstat(path, &st)) return 0; diff --git a/object-store.h b/object-store.h index 5222ee54600..218ffe73604 100644 --- a/object-store.h +++ b/object-store.h @@ -141,7 +141,7 @@ struct packed_git { struct multi_pack_index; -static inline int pack_map_entry_cmp(const void *unused_cmp_data, +static inline int pack_map_entry_cmp(const void *unused_cmp_data UNUSED, const struct hashmap_entry *entry, const struct hashmap_entry *entry2, const void *keydata)
On Fri, Aug 19, 2022 at 10:58:08PM +0200, Ævar Arnfjörð Bjarmason wrote: > Yes, I spoke too soon, sorry. We still need ((unused)). FWIW the below > on top of master and doing: Right. Using ((deprecated)) is really just a substitute for the variable renaming part. And I agree it works as advertised, though I think I prefer the variable-renaming version. One, it feels like we're abusing the deprecated attribute here. The confusion in the compiler output I'm OK with, because we get a chance to put our own message there (so I agree the output is actually better than with my patch). But from time to time I've had to build with -Wno-deprecated-declarations to get around _actual_ deprecated warnings (e.g., compiling with OPENSSL_SHA1=Yes). And doing so would be cutting out half the protection of UNUSED() in that case. Likewise, one thing I like about the renaming is that it fails compilation regardless of -Werror. So it will be caught in any compile, no matter what. And I do automatically compile without DEVELOPER=1 when on a detached HEAD, because historical commits often trigger warnings. Go back far enough and OPENSSL_SHA1 was the default, which generates lots of warnings these days. :) And finally, I actually prefer the parentheses of: static int register_ref(const char *refname, const struct object_id *oid, int UNUSED(flags), void *UNUSED(cb_data)) It visually binds the attribute more tightly to the variable name, like how we sometimes write unused_flags manually in existing code. When there are a lot of variables to mark in a function (and some callbacks really do have a lot), that makes it easier to see what's going on. To me, anyway. I recognize that it's a matter of taste. Technically this last thing is orthogonal to using the deprecated attribute. It could still be used with the parenthesized form, but the error messages gcc generates are horrendous then (it repeats the warning several times due to the macro). So I dunno. These are all matters of opinion, and if it was just my patches, I'd say my taste wins. But all of us are going to have to write these annotations at some time or another when we add callbacks, etc. So we should at the very least pick a syntax the majority prefers. :) -Peff
Jeff King <peff@peff.net> writes: > Likewise, one thing I like about the renaming is that it fails > compilation regardless of -Werror. Yes, I like that aspect of the macro very much. > And finally, I actually prefer the parentheses of: > > static int register_ref(const char *refname, const struct object_id *oid, > int UNUSED(flags), void *UNUSED(cb_data)) That, too. > So I dunno. These are all matters of opinion, and if it was just my > patches, I'd say my taste wins. But all of us are going to have to write > these annotations at some time or another when we add callbacks, etc. So > we should at the very least pick a syntax the majority prefers. :) Well, we can teach others a good taste ;-)
On 8/20/2022 5:46 AM, Jeff King wrote: > On Fri, Aug 19, 2022 at 10:58:08PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Yes, I spoke too soon, sorry. We still need ((unused)). FWIW the below >> on top of master and doing: > > Right. Using ((deprecated)) is really just a substitute for the variable > renaming part. > > And I agree it works as advertised, though I think I prefer the > variable-renaming version. > > One, it feels like we're abusing the deprecated attribute here. The > confusion in the compiler output I'm OK with, because we get a chance to > put our own message there (so I agree the output is actually better than > with my patch). But from time to time I've had to build with > -Wno-deprecated-declarations to get around _actual_ deprecated warnings > (e.g., compiling with OPENSSL_SHA1=Yes). And doing so would be cutting > out half the protection of UNUSED() in that case. The fact that we can't turn on -Wno-deprecated-declarations is enough to convince me that we need to use the variable renaming trick. Thanks, -Stolee
On Sat, Aug 20 2022, Jeff King wrote: > On Fri, Aug 19, 2022 at 10:58:08PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Yes, I spoke too soon, sorry. We still need ((unused)). FWIW the below >> on top of master and doing: > > Right. Using ((deprecated)) is really just a substitute for the variable > renaming part. > > And I agree it works as advertised, though I think I prefer the > variable-renaming version. > > One, it feels like we're abusing the deprecated attribute here. The Definitely, but structurally it seems like a better pick. I.e. isn't the only problem with it the "deprecated" and its interaction with -Wno-deprecated. If the exact same feature existed as a "insert-custom-warning", which would work exactly "deprecated" without the default warning "prefix" would you think this would fit perfectly? > ... > -Wno-deprecated-declarations to get around _actual_ deprecated warnings > (e.g., compiling with OPENSSL_SHA1=Yes). And doing so would be cutting > out half the protection of UNUSED() in that case. This is mildly annoying, but I don't really think it's a practical issue. We're talking about running this without -Wno-deprecated-declarations in CI, and by default. For unused parameters it's enough that we're catching them somewhere, or in common compilation settings, we don't need to catch them *everywhere*, do we? IOW is anyone writing patches where they're testing with -Wno-deprecated-declarations *and* adding unused parameters *and* won't test without -Wno-deprecated-declarations before submitting them, *and* nobody else will catch it? > Likewise, one thing I like about the renaming is that it fails > compilation regardless of -Werror. So it will be caught in any compile, > no matter what. And I do automatically compile without DEVELOPER=1 when > on a detached HEAD, because historical commits often trigger warnings. > Go back far enough and OPENSSL_SHA1 was the default, which generates > lots of warnings these days. :) *nod*, I think this also goes the other way. It's nice to be able to use DEVOPTS=no-error to "get past" various minor issues. I consider an unused parameter as being a minor issue. E.g. when ad-hoc cherry-picking something to test on an older version it can be annoying to have to make larger changes when a DEVOPTS=no-error would do. > And finally, I actually prefer the parentheses of: > > static int register_ref(const char *refname, const struct object_id *oid, > int UNUSED(flags), void *UNUSED(cb_data)) ...and now to the real reason for the follow-up. You/Junio were CC'd, but this is breaking coccinelle, see: https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/ So, bikeshedding-wise I don't care to argue the point. But between odd syntax highlighting and now one analysis tool barfing on it it's a bit more than a bikeshed :)
On Thu, Aug 25, 2022 at 01:00:19PM +0200, Ævar Arnfjörð Bjarmason wrote: > > One, it feels like we're abusing the deprecated attribute here. The > > Definitely, but structurally it seems like a better pick. I.e. isn't the > only problem with it the "deprecated" and its interaction with > -Wno-deprecated. > > If the exact same feature existed as a "insert-custom-warning", which > would work exactly "deprecated" without the default warning "prefix" > would you think this would fit perfectly? Yeah, I agree that would remove my complaint about overloading the meaning. I don't think that exists, though. > This is mildly annoying, but I don't really think it's a practical > issue. We're talking about running this without > -Wno-deprecated-declarations in CI, and by default. > > For unused parameters it's enough that we're catching them somewhere, or > in common compilation settings, we don't need to catch them > *everywhere*, do we? No, but the farther away you go from the edit-compile-run cycle, the more painful warnings become. Catching them immediately and fully has real value, as it means the cost of correcting them is lower. So all things being equal, I think we should prefer universal solutions when they're available (and for example compiler errors over say, coccinelle or other analysis tools). (And yes, I know all things sadly aren't equal; see below...) > IOW is anyone writing patches where they're testing with > -Wno-deprecated-declarations *and* adding unused parameters *and* won't > test without -Wno-deprecated-declarations before submitting them, *and* > nobody else will catch it? Probably not. I don't actually build with -Wno-deprecated-declarations routinely. But my fear is that some platform may be stuck there for a while (because an overzealous libc marks something). But that's kind of hypothetical, so we may have to just accept it and cross that bridge if we come to it. > > And finally, I actually prefer the parentheses of: > > > > static int register_ref(const char *refname, const struct object_id *oid, > > int UNUSED(flags), void *UNUSED(cb_data)) > > ...and now to the real reason for the follow-up. You/Junio were CC'd, > but this is breaking coccinelle, see: > https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/ Ugh. Yeah, that is really unfortunate. I much prefer the parenthesized syntax, but if we can't find a way to unconfuse third-party parsing, then switching is probably the least-bad solution. -Peff
Hi Peff On Fri, 26 Aug 2022 at 08:33, Jeff King <peff@peff.net> wrote: > > On Thu, Aug 25, 2022 at 01:00:19PM +0200, Ævar Arnfjörð Bjarmason wrote: > > > > One, it feels like we're abusing the deprecated attribute here. The > > > > Definitely, but structurally it seems like a better pick. I.e. isn't the > > only problem with it the "deprecated" and its interaction with > > -Wno-deprecated. > > This is mildly annoying, but I don't really think it's a practical > > issue. We're talking about running this without > > -Wno-deprecated-declarations in CI, and by default. > > > > For unused parameters it's enough that we're catching them somewhere, or > > in common compilation settings, we don't need to catch them > > *everywhere*, do we? > > No, but the farther away you go from the edit-compile-run cycle, the > more painful warnings become. Catching them immediately and fully has > real value, as it means the cost of correcting them is lower. So all > things being equal, I think we should prefer universal solutions when > they're available (and for example compiler errors over say, coccinelle > or other analysis tools). That's a good point, one of the nice things about your macro was that all compilers detected when UNUSED() parameters were in fact used. In comparison abusing the deprecated attribute is uglier and less practical. > (And yes, I know all things sadly aren't equal; see below...) It might be worth reporting this issue on the coccinelle mailing list to see if anyone is interested in fixing it. If it gets fixed we're left with the problem of having to build it for our ci but that shouldn't be insurmountable. Best Wishes Phillip > > IOW is anyone writing patches where they're testing with > > -Wno-deprecated-declarations *and* adding unused parameters *and* won't > > test without -Wno-deprecated-declarations before submitting them, *and* > > nobody else will catch it? > > Probably not. I don't actually build with -Wno-deprecated-declarations > routinely. But my fear is that some platform may be stuck there for a > while (because an overzealous libc marks something). But that's kind of > hypothetical, so we may have to just accept it and cross that bridge if > we come to it. > > > > And finally, I actually prefer the parentheses of: > > > > > > static int register_ref(const char *refname, const struct object_id *oid, > > > int UNUSED(flags), void *UNUSED(cb_data)) > > > > ...and now to the real reason for the follow-up. You/Junio were CC'd, > > but this is breaking coccinelle, see: > > https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/ > > Ugh. Yeah, that is really unfortunate. I much prefer the parenthesized > syntax, but if we can't find a way to unconfuse third-party parsing, > then switching is probably the least-bad solution. > > -Peff
Jeff King <peff@peff.net> writes: > No, but the farther away you go from the edit-compile-run cycle, the > more painful warnings become. Catching them immediately and fully has > real value, as it means the cost of correcting them is lower. So all > things being equal, I think we should prefer universal solutions when > they're available (and for example compiler errors over say, coccinelle > or other analysis tools). Thanks for saying it so succinctly. Making compiler errors less useful only to please Coccinelle is putting our priority wrong. > Ugh. Yeah, that is really unfortunate. I much prefer the parenthesized > syntax, but if we can't find a way to unconfuse third-party parsing, > then switching is probably the least-bad solution. Or have third-party fix the parsing ;-) Until then, perhaps we have to live with a suboptimal syntax. Thanks.
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/