patch, 52 partsfix some -Wmissing-field-initializer warnings

[No, I won't be sending 52 patches to the list!]

This series, started last year, has been hanging around because the time never seemed right to send it to the list. It may still not be the right time ... :-D

I would like to be able to compile git using '-Wall -Wextra -Werror' compiler options. In order to see how far away we are from that possibility, you can build with DEVELOPER=1 along with the additional settings: 'DEVOPTS=extra-all no-error pedantic'.

That leads to many warnings:

      $ git describe
      v2.22.0-rc1
      $ make >out 2>&1
      $ grep warning out | sed -e 's/.*\[-W/\[-W/' | sort | uniq -c
            9 [-Wempty-body]
         1694 [-Wmissing-field-initializers]
          159 [-Wpedantic]
          945 [-Wsign-compare]
         2821 [-Wunused-parameter]
      $

Note that at the beginning of this cycle, the numbers were quite a bit smaller (and this series was only 37 patches, rather than 52):

      $ git describe
      v2.21.0
      $ make >out 2>&1
      $ grep warning out | sed -e 's/.*\[-W/\[-W/' | sort | uniq -c
            9 [-Wempty-body]
          759 [-Wmissing-field-initializers]
          925 [-Wsign-compare]
         2732 [-Wunused-parameter]
      $

This series removes the, newly introduced, pedantic warnings and eliminates all 1371 'missing-field-initializers' warnings that relate to 'struct option':

      $ cat out-warn-master.stats
            9 [-Wempty-body]
          323 [-Wmissing-field-initializers]
          945 [-Wsign-compare]
         2821 [-Wunused-parameter]
      $ 

Thus, after about six months, I am further away from my target than when I started! ;-)

This series does not fix any problems or add any new features, so it is not important (hence the tendency to 'slip'). I don't want to flood the mailing list with patches that nobody wants, so: is there any interest in these kinds of patches? If not, I will stop now! (I have a 2-3 year old branch that addressed the '-Wsign-compare' warnings, but that is probably beyond salvaging by now :( ).

This series is available from: git://repo.or.cz/git/raj.git with the branch name 'warn-master'. A trial merge to current 'next' and 'pu' branches can be found at 'warn-next' and 'warn-pu' branches. (The merge to 'next' went without problem, and 'pu' only required a fixup to the builtin/commit patch).

[The 'warn-v2.21' branch shows the previous version of the series.]

What do you think?

Thanks!

ATB, Ramsay Jones

Ramsay Jones (52):
  parse-options: reformat the OPT_X() macros
  parse-options: move some one-line OPT_X() macros
  parse-options: rename callback parameter from 'f' to 'cb'
  parse-options: add missing field initializers
  list-objects-filter-options: add missing initializer
  ref-filter.h: add missing field initializers
  parse-options: add an OPT_LL_CALLBACK() macro
  parse-options.h: add 'd' parameter to OPT_INTEGER_F()
  parse-options.h: fix some -Wpedantic warnings
  builtin/update-index: fix some -Wmissing-field-initializers warnings
  builtin/log: fix some -Wmissing-field-initializers warnings
  builtin/notes: fix some -Wmissing-field-initializers warnings
  builtin/grep: fix some -Wmissing-field-initializers warnings
  builtin/commit: fix some -Wmissing-field-initializers warnings
  apply.c: fix some -Wmissing-field-initializers warnings
  builtin/rebase: fix some -Wmissing-field-initializers warnings
  builtin/config: add missing field initializers
  test-parse-options: fix some -Wmissing-field-initializers warnings
  builtin/read-tree: fix some -Wmissing-field-initializers warnings
  builtin/merge: fix some -Wmissing-field-initializers warnings
  builtin/fetch: fix some -Wmissing-field-initializers warnings
  builtin/commit-tree: fix some -Wmissing-field-initializers warnings
  builtin/tag: fix an -Wmissing-field-initializers warning
  builtin/push: fix some -Wmissing-field-initializers warnings
  builtin/pack-objects: fix some -Wmissing-field-initializers warnings
  builtin/ls-files: fix some -Wmissing-field-initializers warnings
  builtin/blame: fix some -Wmissing-field-initializers warnings
  builtin/show-ref: fix some -Wmissing-field-initializers warnings
  builtin/show-branch: fix an -Wmissing-field-initializers warning
  builtin/send-pack: fix some -Wmissing-field-initializers warnings
  builtin/pull: fix some -Wmissing-field-initializers warnings
  builtin/fmt-merge-msg: fix some -Wmissing-field-initializers warnings
  builtin/describe: fix some -Wmissing-field-initializers warnings
  builtin/cat-file: fix some -Wmissing-field-initializers warnings
  builtin/shortlog: fix an -Wmissing-field-initializers warning
  builtin/reset: fix an -Wmissing-field-initializers warning
  builtin/remote: fix an -Wmissing-field-initializers warning
  builtin/ls-remote: fix an -Wmissing-field-initializers warning
  builtin/interpret-trailers: fix an -Wmissing-field-initializers warning
  builtin/clean: fix an -Wmissing-field-initializers warning
  builtin/checkout-index: fix an -Wmissing-field-initializers warning
  builtin/checkout: fix an -Wmissing-field-initializers warning
  builtin/branch: fix an -Wmissing-field-initializers warning
  builtin/add: fix an -Wmissing-field-initializers warning
  builtin/write-tree: fix an -Wmissing-field-initializers warning
  builtin/revert: fix an -Wmissing-field-initializers warning
  builtin/name-rev: fix an -Wmissing-field-initializers warning
  builtin/init-db: fix an -Wmissing-field-initializers warning
  builtin/gc: fix an -Wmissing-field-initializers warning
  builtin/clone: fix an -Wmissing-field-initializers warning
  builtin/am: fix an -Wmissing-field-initializers warning
  diff.c: fix an -Wmissing-field-initializers warning

 apply.c                       |  28 ++---
 builtin/add.c                 |   4 +-
 builtin/am.c                  |   6 +-
 builtin/blame.c               |   6 +-
 builtin/branch.c              |   7 +-
 builtin/cat-file.c            |   8 +-
 builtin/checkout-index.c      |   4 +-
 builtin/checkout.c            |   7 +-
 builtin/clean.c               |   5 +-
 builtin/clone.c               |   2 +-
 builtin/commit-tree.c         |  24 ++--
 builtin/commit.c              |  21 ++--
 builtin/config.c              |   2 +-
 builtin/describe.c            |   8 +-
 builtin/fetch.c               |  17 +--
 builtin/fmt-merge-msg.c       |  10 +-
 builtin/gc.c                  |   3 +-
 builtin/grep.c                |  24 ++--
 builtin/init-db.c             |   4 +-
 builtin/interpret-trailers.c  |   4 +-
 builtin/log.c                 |  56 ++++-----
 builtin/ls-files.c            |  12 +-
 builtin/ls-remote.c           |   4 +-
 builtin/merge.c               |  17 +--
 builtin/name-rev.c            |   8 +-
 builtin/notes.c               |  32 ++---
 builtin/pack-objects.c        |  12 +-
 builtin/pull.c                |  16 +--
 builtin/push.c                |  22 ++--
 builtin/read-tree.c           |  19 +--
 builtin/rebase.c              |  69 ++++++-----
 builtin/remote.c              |   5 +-
 builtin/reset.c               |   5 +-
 builtin/revert.c              |   5 +-
 builtin/send-pack.c           |  11 +-
 builtin/shortlog.c            |   4 +-
 builtin/show-branch.c         |  10 +-
 builtin/show-ref.c            |  11 +-
 builtin/tag.c                 |  19 ++-
 builtin/update-index.c        |  61 +++++-----
 builtin/write-tree.c          |   6 +-
 diff.c                        |   4 +-
 list-objects-filter-options.h |   2 +-
 parse-options.h               | 216 ++++++++++++++++++++++------------
 ref-filter.h                  |   2 +-
 t/helper/test-parse-options.c |  16 +--
 46 files changed, 458 insertions(+), 380 deletions(-)

On Fri, May 24 2019, Ramsay Jones wrote:

I think just send it to the list. We've seen worse, and it's easier to review than needing to get out of the normal E-Mail workflow.

I'm keen on getting us to stricter compiler warnings in general, but we'll see whether this specific thing seems worth it in review.

On 24/05/2019 23:30, Ævar Arnfjörð Bjarmason wrote:

I forgot to mention that this series has a long tail. The first ten patches (in addition to all pedantic warnings) removes 1246 warnings. The next 10 removes another 70 and the final 32 patches only removes 55 warnings (the last 18 patches remove only one warning each).

Hmm, so maybe I should only send the first few out? 

I will give it some thought (while waiting for some more comments).

Thanks.

ATB, Ramsay Jones

Hi Ramsay,

I had a brief look at the series. IMO, it is a mistake to appease -Wmissing-field-initializer.

We have two sorts of initializers:

 - zero initializers: they just want to null out every field,
   like CHILD_PROCESS_INIT and ad-hoc initializers of structs
   such as xpparam_t pp = { 0 }; in range-diff.c

 - value initializers are always macros, such as STRING_LIST_INIT_DUP
   and the OPT_* family.

I am strongly against forcing zero initializers to write down a value for every field. It is much more preferable to depend on that the compiler does the right thing with them. -Wmissing-field-initializer would provide guidance in the wrong direction. A zero initializer looks like this: = { 0 }; and nothing else.

(And for this reason, I also think it is wrong to change 0 to NULL in initializers to appease sparse's "zero used as pointer value" warning. Let the compiler do the right thing.)

Value initializers are a different matter. The changes you have prepared around struct option initializers are good, IMO. Whether or not you add trailing zeros to the macros makes no difference for the users of the macros.

-- Hannes

On Thu, May 30, 2019 at 10:47:37AM +0200, Johannes Sixt wrote:

I had a similar impression while perusing the commits. I don't mind forcing some extra work on programmers to appease a warning if disregarding it is a common source of errors. But I didn't see any real bug-fixes in the series, so it doesn't seem like that good a tradeoff to me.

Contrast that with the -Wunused-parameters warning. I found a dozen or so actual bugs by sifting through the results, and another couple dozen spots where the code could be cleaned up or simplified. If we want to shut up the warning completely (so we can pay attention to it), we'll then have to annotate probably a couple hundred spots, and keep those annotations up to date. But I feel better doing that knowing that it's shown real-world value.

-Peff

On 30/05/2019 13:04, Jeff King wrote:

OK, I will drop this branch then.

Thanks all.

ATB, Ramsay Jones