| Message ID | 20190320081258.GA5621@sigill.intra.peff.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | more unused parameter cleanups | expand |
Jeff King <peff@peff.net> writes: > Here's another round of -Wunused-parameter cleanups. Previous rounds > were at [1] and [2]. As before, these are mostly just removals, so it's > easy to see there's no behavior change (there are a couple of cleanups > that ride along, though, so watch for those). > > There are two minor conflicts when merging with pu: > > - jt/fetch-cdn-offload tweaked the "pack_lockfile" parameter to > fetch_pack(). A few other parameters are dropped in this series. > The textual resolution is pretty straightforward. > > - ps/stash-in-c (et al) added a new call to report_path_error() in > builtin/stash.c, which here loses its redundant "prefix" parameter. > There's no textual conflict, but the new call needs to drop its > final NULL parameter in order to compile. > > [1] https://public-inbox.org/git/20190214054736.GA20091@sigill.intra.peff.net/ > [2] https://public-inbox.org/git/20190124131104.GA24017@sigill.intra.peff.net/ > > The patches are: > > [01/13]: revision: drop some unused "revs" parameters > [02/13]: log: drop unused rev_info from early output > [03/13]: log: drop unused "len" from show_tagger() > [04/13]: update-index: drop unused prefix_length parameter from do_reupdate() > [05/13]: test-date: drop unused "now" parameter from parse_dates() > [06/13]: unpack-trees: drop name_entry from traverse_by_cache_tree() > [07/13]: unpack-trees: drop unused error_type parameters > [08/13]: report_path_error(): drop unused prefix parameter > [09/13]: fetch_pack(): drop unused parameters > [10/13]: parse-options: drop unused ctx parameter from show_gitcomp() > [11/13]: pretty: drop unused "type" parameter in needs_rfc2047_encoding() > [12/13]: pretty: drop unused strbuf from parse_padding_placeholder() > [13/13]: parse_opt_ref_sorting: always use with NONEG flag Nicely written. Thanks. > builtin/branch.c | 3 +-- > builtin/checkout.c | 2 +- > builtin/commit.c | 6 +++--- > builtin/fetch-pack.c | 2 +- > builtin/for-each-ref.c | 3 +-- > builtin/log.c | 18 +++++++++--------- > builtin/ls-files.c | 2 +- > builtin/ls-remote.c | 3 +-- > builtin/submodule--helper.c | 2 +- > builtin/tag.c | 3 +-- > builtin/update-index.c | 5 ++--- > dir.c | 3 +-- > dir.h | 2 +- > fetch-pack.c | 3 +-- > fetch-pack.h | 3 +-- > parse-options.c | 5 ++--- > pretty.c | 12 +++++------- > ref-filter.c | 9 +++++++-- > ref-filter.h | 5 +++++ > revision.c | 12 ++++++------ > t/helper/test-date.c | 4 ++-- > transport.c | 10 ++++------ > unpack-trees.c | 9 +++------ > 23 files changed, 60 insertions(+), 66 deletions(-) > > -Peff
On Wed, Mar 20 2019, Jeff King wrote: > Here's another round of -Wunused-parameter cleanups. Previous rounds > were at [1] and [2]. As before, these are mostly just removals, so it's > easy to see there's no behavior change (there are a couple of cleanups > that ride along, though, so watch for those). > > There are two minor conflicts when merging with pu: > > - jt/fetch-cdn-offload tweaked the "pack_lockfile" parameter to > fetch_pack(). A few other parameters are dropped in this series. > The textual resolution is pretty straightforward. > > - ps/stash-in-c (et al) added a new call to report_path_error() in > builtin/stash.c, which here loses its redundant "prefix" parameter. > There's no textual conflict, but the new call needs to drop its > final NULL parameter in order to compile. LGTM from skimming it, FWIW this is now what we need to compile cleanly with -Wextra: make DEVELOPER=1 DEVOPTS="extra-all" CFLAGS="-Wno-unused-parameter -Wno-missing-field-initializers -Wno-sign-compare -Wno-empty-body" all For some such as -Wempty-body we'd really need to contort ourselves to get it passing anywhere near cleanly (all of those have existing "/* this is intentional! */" comments). I wonder if for the rest of these it's worth re-picking up this old suggestions of yours about #pragma: https://public-inbox.org/git/20170126143252.ne533mcv3n2ksbai@sigill.intra.peff.net/ I.e. for us to define our own macro for these cases & use it. https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas I was looking into this for SunCC the other day, it has various new warnings that are useful that neither gcc nor clang (or anything else I've tried) has, but also has some stupidities due to faulty code analysis, luckily those can be disabled: https://docs.oracle.com/cd/E19205-01/819-5265/bjaby/index.html This would allow me to compile there with -Werror. It would mean quite some macro verbosity in some existing code, maybe it's not worth it...
On Thu, Mar 21, 2019 at 09:50:13AM +0100, Ævar Arnfjörð Bjarmason wrote: > LGTM from skimming it, FWIW this is now what we need to compile cleanly > with -Wextra: > > make DEVELOPER=1 DEVOPTS="extra-all" CFLAGS="-Wno-unused-parameter -Wno-missing-field-initializers -Wno-sign-compare -Wno-empty-body" all > > For some such as -Wempty-body we'd really need to contort ourselves to > get it passing anywhere near cleanly (all of those have existing "/* > this is intentional! */" comments). I think we could probably define a NOOP_BODY macro or function and use that instead. But it may not be worth the trouble. I'd have to see how painful that would be, and whether it might find any cases that actually look like real bugs. For -Wunused-parameter I am working towards being able to actually enable that everywhere. It is not _too_ bad to annotate the instances which must be there, and my digging with it has uncovered several real bugs. Right now I'm in the "drop useless parameters" phase, which I expect will take one or two more rounds. Then I'll start on the "annotate unused ones we must keep" series, which culminates in actually flipping on the switch with DEVELOPER (or rather, stopping flipping it off). You can see my progress on the jk/unused-4-mark branch of https://github.com/peff/git (I think the contents are good, but the commit messages and organization need some cleanup). > I wonder if for the rest of these it's worth re-picking up this old > suggestions of yours about #pragma: > https://public-inbox.org/git/20170126143252.ne533mcv3n2ksbai@sigill.intra.peff.net/ > > I.e. for us to define our own macro for these cases & use it. The push/pop ones may be of use (which both clang and gcc seem to support), since that would let us localize the effects. I think in many cases there's usually a more readable solution, though (e.g., you'd want to annotate specific parameters as unused with single word, not a 3-line push-diag/declare-param/pop-diag mess). -Peff
Here's another round of -Wunused-parameter cleanups. Previous rounds were at [1] and [2]. As before, these are mostly just removals, so it's easy to see there's no behavior change (there are a couple of cleanups that ride along, though, so watch for those). There are two minor conflicts when merging with pu: - jt/fetch-cdn-offload tweaked the "pack_lockfile" parameter to fetch_pack(). A few other parameters are dropped in this series. The textual resolution is pretty straightforward. - ps/stash-in-c (et al) added a new call to report_path_error() in builtin/stash.c, which here loses its redundant "prefix" parameter. There's no textual conflict, but the new call needs to drop its final NULL parameter in order to compile. [1] https://public-inbox.org/git/20190214054736.GA20091@sigill.intra.peff.net/ [2] https://public-inbox.org/git/20190124131104.GA24017@sigill.intra.peff.net/ The patches are: [01/13]: revision: drop some unused "revs" parameters [02/13]: log: drop unused rev_info from early output [03/13]: log: drop unused "len" from show_tagger() [04/13]: update-index: drop unused prefix_length parameter from do_reupdate() [05/13]: test-date: drop unused "now" parameter from parse_dates() [06/13]: unpack-trees: drop name_entry from traverse_by_cache_tree() [07/13]: unpack-trees: drop unused error_type parameters [08/13]: report_path_error(): drop unused prefix parameter [09/13]: fetch_pack(): drop unused parameters [10/13]: parse-options: drop unused ctx parameter from show_gitcomp() [11/13]: pretty: drop unused "type" parameter in needs_rfc2047_encoding() [12/13]: pretty: drop unused strbuf from parse_padding_placeholder() [13/13]: parse_opt_ref_sorting: always use with NONEG flag builtin/branch.c | 3 +-- builtin/checkout.c | 2 +- builtin/commit.c | 6 +++--- builtin/fetch-pack.c | 2 +- builtin/for-each-ref.c | 3 +-- builtin/log.c | 18 +++++++++--------- builtin/ls-files.c | 2 +- builtin/ls-remote.c | 3 +-- builtin/submodule--helper.c | 2 +- builtin/tag.c | 3 +-- builtin/update-index.c | 5 ++--- dir.c | 3 +-- dir.h | 2 +- fetch-pack.c | 3 +-- fetch-pack.h | 3 +-- parse-options.c | 5 ++--- pretty.c | 12 +++++------- ref-filter.c | 9 +++++++-- ref-filter.h | 5 +++++ revision.c | 12 ++++++------ t/helper/test-date.c | 4 ++-- transport.c | 10 ++++------ unpack-trees.c | 9 +++------ 23 files changed, 60 insertions(+), 66 deletions(-) -Peff