mbox series

[0/9] drop some unused parameters

Message ID 20190124131104.GA24017@sigill.intra.peff.net (mailing list archive)
Headers show
Series drop some unused parameters | expand

Message

Jeff King Jan. 24, 2019, 1:11 p.m. UTC
I've mentioned before that I have a series which compiles cleanly with
-Wunused-parameters. I split this work roughly into three groups:

  1. Patches that fix bugs (i.e., we should have been using the
     parameter but didn't).

  2. Patches that drop unused parameters (i.e., code cleanup).

  3. Patches that annotate undroppable parameters (e.g., ones that are
     present due to a callback interface).

All of the patches from group 1 have been merged already. So this series
starts us off on group 2. There are about 50 patches in that group.
Given that none of them are urgent, I plan to feed them in batches to
avoid overwhelming reviewers. I'm also ordering them to avoid conflicts
with other topics in flight (this batch has no conflicts with 'next',
and only one minor textual conflict with 'pu').

The patches themselves are pretty much independent from each other. I
based these on master. They're cleanup which _could_ go to maint, and I
suspect they'd apply there, too (most of these are pretty old), but
again, I don't think there's a particular urgency to this.

  [1/8]: match-trees: drop unused path parameter from score functions
  [2/8]: apply: drop unused "def" parameter from find_name_gnu()
  [3/8]: create_bundle(): drop unused "header" parameter
  [4/8]: column: drop unused "opts" parameter in item_length()
  [5/8]: show_date_relative(): drop unused "tz" parameter
  [6/8]: config: drop unused parameter from maybe_remove_section()
  [7/8]: convert: drop len parameter from conversion checks
  [8/8]: convert: drop path parameter from actual conversion functions

 apply.c              |  5 ++---
 builtin/bundle.c     |  3 +--
 bundle.c             |  4 ++--
 bundle.h             |  4 ++--
 cache.h              |  2 +-
 column.c             |  4 ++--
 config.c             |  3 +--
 convert.c            | 28 ++++++++++++++--------------
 date.c               |  8 ++++----
 match-trees.c        | 16 +++++++---------
 t/helper/test-date.c |  2 +-
 11 files changed, 37 insertions(+), 42 deletions(-)

Comments

Derrick Stolee Jan. 24, 2019, 2:09 p.m. UTC | #1
On 1/24/2019 8:11 AM, Jeff King wrote:
>    2. Patches that drop unused parameters (i.e., code cleanup).
I've read the patches and they do exactly this, along with explanations 
of why they don't fit in the first category.

Thanks,
-Stolee
Junio C Hamano Jan. 24, 2019, 8:35 p.m. UTC | #2
Jeff King <peff@peff.net> writes:

> The patches themselves are pretty much independent from each other. I
> based these on master. They're cleanup which _could_ go to maint, and I
> suspect they'd apply there, too (most of these are pretty old), but
> again, I don't think there's a particular urgency to this.

Thanks for being considerate.  One potential benefit that we might
gain by applying these clean-ups also to 'maint' is that it would
allow us to keep 'master' and 'maint' more in sync, so it would make
it more likely for unrelated bugfixes we may have in the future in
the vicinity to also apply cleanly to both.  By definition, the
variables removed are not used, so it would make an interesting
situation if such a future bugfix needs to look at them (sort of
turning the changes in this series into category 1 "we should have
been looking at them, but we didn't"), but at that point, we'd be
resurrecting these variables we drop here, so it won't be really
hurting us.  And if such a bugfix does not need to look at them,
then textual context that might be caused by not applying these to
'maint' but keeping them only to 'master' would not be a big deal,
either.

So I can go either way, but I am tempted to leave them outside
'maint' --- at least that is my current thinking.

Thanks.

>
>   [1/8]: match-trees: drop unused path parameter from score functions
>   [2/8]: apply: drop unused "def" parameter from find_name_gnu()
>   [3/8]: create_bundle(): drop unused "header" parameter
>   [4/8]: column: drop unused "opts" parameter in item_length()
>   [5/8]: show_date_relative(): drop unused "tz" parameter
>   [6/8]: config: drop unused parameter from maybe_remove_section()
>   [7/8]: convert: drop len parameter from conversion checks
>   [8/8]: convert: drop path parameter from actual conversion functions
>
>  apply.c              |  5 ++---
>  builtin/bundle.c     |  3 +--
>  bundle.c             |  4 ++--
>  bundle.h             |  4 ++--
>  cache.h              |  2 +-
>  column.c             |  4 ++--
>  config.c             |  3 +--
>  convert.c            | 28 ++++++++++++++--------------
>  date.c               |  8 ++++----
>  match-trees.c        | 16 +++++++---------
>  t/helper/test-date.c |  2 +-
>  11 files changed, 37 insertions(+), 42 deletions(-)
brian m. carlson Jan. 25, 2019, 1:53 a.m. UTC | #3
On Thu, Jan 24, 2019 at 08:11:05AM -0500, Jeff King wrote:
> I've mentioned before that I have a series which compiles cleanly with
> -Wunused-parameters. I split this work roughly into three groups:
> 
>   1. Patches that fix bugs (i.e., we should have been using the
>      parameter but didn't).
> 
>   2. Patches that drop unused parameters (i.e., code cleanup).
> 
>   3. Patches that annotate undroppable parameters (e.g., ones that are
>      present due to a callback interface).
> 
> All of the patches from group 1 have been merged already. So this series
> starts us off on group 2. There are about 50 patches in that group.
> Given that none of them are urgent, I plan to feed them in batches to
> avoid overwhelming reviewers. I'm also ordering them to avoid conflicts
> with other topics in flight (this batch has no conflicts with 'next',
> and only one minor textual conflict with 'pu').

All of these patches seemed straightforward. I did see a few where there
may have originally been some consistency benefit to keeping parameters
(always passing file name to *_to_worktree, for example), but I'm fine
dropping them if it means we can get better development help from the
compiler.

I am also, for the record, in favor of ignoring the effects of
relativity for the purposes of Git. :)