{"thread":{"id":"51172","subject":"[PATCH 00/52] fix some -Wmissing-field-initializer warnings","startedAt":"2019-05-24T20:28:27Z","lastAt":"2019-05-30T14:59:13Z","messageCount":6,"participants":["Ramsay Jones","Ævar Arnfjörð Bjarmason","Johannes Sixt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":52},"messages":[{"id":"376204","messageId":"cf0b720e-ad29-79e7-17d5-b69d50e7007e@ramsayjones.plus.com","threadId":"51172","inReplyTo":null,"subject":"[PATCH 00/52] fix some -Wmissing-field-initializer warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-05-24T20:28:20Z","receivedAt":"2019-05-24T20:28:27Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n[No, I won't be sending 52 patches to the list!]\n\nThis series, started last year, has been hanging around because the\ntime never seemed right to send it to the list. It may still not be\nthe right time ... :-D\n\nI would like to be able to compile git using '-Wall -Wextra -Werror'\ncompiler options. In order to see how far away we are from that\npossibility, you can build with DEVELOPER=1 along with the additional\nsettings: 'DEVOPTS=extra-all no-error pedantic'.\n\nThat leads to many warnings:\n\n      $ git describe\n      v2.22.0-rc1\n      $ make >out 2>&1\n      $ grep warning out | sed -e 's/.*\\[-W/\\[-W/' | sort | uniq -c\n            9 [-Wempty-body]\n         1694 [-Wmissing-field-initializers]\n          159 [-Wpedantic]\n          945 [-Wsign-compare]\n         2821 [-Wunused-parameter]\n      $\n\nNote that at the beginning of this cycle, the numbers were quite a bit\nsmaller (and this series was only 37 patches, rather than 52):\n\n      $ git describe\n      v2.21.0\n      $ make >out 2>&1\n      $ grep warning out | sed -e 's/.*\\[-W/\\[-W/' | sort | uniq -c\n            9 [-Wempty-body]\n          759 [-Wmissing-field-initializers]\n          925 [-Wsign-compare]\n         2732 [-Wunused-parameter]\n      $\n\nThis series removes the, newly introduced, pedantic warnings and\neliminates all 1371 'missing-field-initializers' warnings that\nrelate to 'struct option':\n\n      $ cat out-warn-master.stats\n            9 [-Wempty-body]\n          323 [-Wmissing-field-initializers]\n          945 [-Wsign-compare]\n         2821 [-Wunused-parameter]\n      $ \n\nThus, after about six months, I am further away from my target than\nwhen I started! ;-)\n\nThis series does not fix any problems or add any new features, so it\nis not important (hence the tendency to 'slip'). I don't want to\nflood the mailing list with patches that nobody wants, so: is there\nany interest in these kinds of patches? If not, I will stop now!\n(I have a 2-3 year old branch that addressed the '-Wsign-compare'\nwarnings, but that is probably beyond salvaging by now :( ).\n\nThis series is available from: git://repo.or.cz/git/raj.git with the\nbranch name 'warn-master'. A trial merge to current 'next' and 'pu'\nbranches can be found at 'warn-next' and 'warn-pu' branches. (The\nmerge to 'next' went without problem, and 'pu' only required a fixup\nto the builtin/commit patch).\n\n[The 'warn-v2.21' branch shows the previous version of the series.]\n\nWhat do you think?\n\nThanks!\n\nATB,\nRamsay Jones\n\nRamsay Jones (52):\n  parse-options: reformat the OPT_X() macros\n  parse-options: move some one-line OPT_X() macros\n  parse-options: rename callback parameter from 'f' to 'cb'\n  parse-options: add missing field initializers\n  list-objects-filter-options: add missing initializer\n  ref-filter.h: add missing field initializers\n  parse-options: add an OPT_LL_CALLBACK() macro\n  parse-options.h: add 'd' parameter to OPT_INTEGER_F()\n  parse-options.h: fix some -Wpedantic warnings\n  builtin/update-index: fix some -Wmissing-field-initializers warnings\n  builtin/log: fix some -Wmissing-field-initializers warnings\n  builtin/notes: fix some -Wmissing-field-initializers warnings\n  builtin/grep: fix some -Wmissing-field-initializers warnings\n  builtin/commit: fix some -Wmissing-field-initializers warnings\n  apply.c: fix some -Wmissing-field-initializers warnings\n  builtin/rebase: fix some -Wmissing-field-initializers warnings\n  builtin/config: add missing field initializers\n  test-parse-options: fix some -Wmissing-field-initializers warnings\n  builtin/read-tree: fix some -Wmissing-field-initializers warnings\n  builtin/merge: fix some -Wmissing-field-initializers warnings\n  builtin/fetch: fix some -Wmissing-field-initializers warnings\n  builtin/commit-tree: fix some -Wmissing-field-initializers warnings\n  builtin/tag: fix an -Wmissing-field-initializers warning\n  builtin/push: fix some -Wmissing-field-initializers warnings\n  builtin/pack-objects: fix some -Wmissing-field-initializers warnings\n  builtin/ls-files: fix some -Wmissing-field-initializers warnings\n  builtin/blame: fix some -Wmissing-field-initializers warnings\n  builtin/show-ref: fix some -Wmissing-field-initializers warnings\n  builtin/show-branch: fix an -Wmissing-field-initializers warning\n  builtin/send-pack: fix some -Wmissing-field-initializers warnings\n  builtin/pull: fix some -Wmissing-field-initializers warnings\n  builtin/fmt-merge-msg: fix some -Wmissing-field-initializers warnings\n  builtin/describe: fix some -Wmissing-field-initializers warnings\n  builtin/cat-file: fix some -Wmissing-field-initializers warnings\n  builtin/shortlog: fix an -Wmissing-field-initializers warning\n  builtin/reset: fix an -Wmissing-field-initializers warning\n  builtin/remote: fix an -Wmissing-field-initializers warning\n  builtin/ls-remote: fix an -Wmissing-field-initializers warning\n  builtin/interpret-trailers: fix an -Wmissing-field-initializers warning\n  builtin/clean: fix an -Wmissing-field-initializers warning\n  builtin/checkout-index: fix an -Wmissing-field-initializers warning\n  builtin/checkout: fix an -Wmissing-field-initializers warning\n  builtin/branch: fix an -Wmissing-field-initializers warning\n  builtin/add: fix an -Wmissing-field-initializers warning\n  builtin/write-tree: fix an -Wmissing-field-initializers warning\n  builtin/revert: fix an -Wmissing-field-initializers warning\n  builtin/name-rev: fix an -Wmissing-field-initializers warning\n  builtin/init-db: fix an -Wmissing-field-initializers warning\n  builtin/gc: fix an -Wmissing-field-initializers warning\n  builtin/clone: fix an -Wmissing-field-initializers warning\n  builtin/am: fix an -Wmissing-field-initializers warning\n  diff.c: fix an -Wmissing-field-initializers warning\n\n apply.c                       |  28 ++---\n builtin/add.c                 |   4 +-\n builtin/am.c                  |   6 +-\n builtin/blame.c               |   6 +-\n builtin/branch.c              |   7 +-\n builtin/cat-file.c            |   8 +-\n builtin/checkout-index.c      |   4 +-\n builtin/checkout.c            |   7 +-\n builtin/clean.c               |   5 +-\n builtin/clone.c               |   2 +-\n builtin/commit-tree.c         |  24 ++--\n builtin/commit.c              |  21 ++--\n builtin/config.c              |   2 +-\n builtin/describe.c            |   8 +-\n builtin/fetch.c               |  17 +--\n builtin/fmt-merge-msg.c       |  10 +-\n builtin/gc.c                  |   3 +-\n builtin/grep.c                |  24 ++--\n builtin/init-db.c             |   4 +-\n builtin/interpret-trailers.c  |   4 +-\n builtin/log.c                 |  56 ++++-----\n builtin/ls-files.c            |  12 +-\n builtin/ls-remote.c           |   4 +-\n builtin/merge.c               |  17 +--\n builtin/name-rev.c            |   8 +-\n builtin/notes.c               |  32 ++---\n builtin/pack-objects.c        |  12 +-\n builtin/pull.c                |  16 +--\n builtin/push.c                |  22 ++--\n builtin/read-tree.c           |  19 +--\n builtin/rebase.c              |  69 ++++++-----\n builtin/remote.c              |   5 +-\n builtin/reset.c               |   5 +-\n builtin/revert.c              |   5 +-\n builtin/send-pack.c           |  11 +-\n builtin/shortlog.c            |   4 +-\n builtin/show-branch.c         |  10 +-\n builtin/show-ref.c            |  11 +-\n builtin/tag.c                 |  19 ++-\n builtin/update-index.c        |  61 +++++-----\n builtin/write-tree.c          |   6 +-\n diff.c                        |   4 +-\n list-objects-filter-options.h |   2 +-\n parse-options.h               | 216 ++++++++++++++++++++++------------\n ref-filter.h                  |   2 +-\n t/helper/test-parse-options.c |  16 +--\n 46 files changed, 458 insertions(+), 380 deletions(-)\n\n-- \n2.21.0\n"},{"id":"376208","messageId":"87a7fbfptk.fsf@evledraar.gmail.com","threadId":"51172","inReplyTo":"cf0b720e-ad29-79e7-17d5-b69d50e7007e@ramsayjones.plus.com","subject":"Re: [PATCH 00/52] fix some -Wmissing-field-initializer warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-24T22:30:31Z","receivedAt":"2019-05-24T22:30:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, May 24 2019, Ramsay Jones wrote:\n\n> [No, I won't be sending 52 patches to the list!]\n> [...]\n> This series does not fix any problems or add any new features, so it\n> is not important (hence the tendency to 'slip'). I don't want to\n> flood the mailing list with patches that nobody wants, so: is there\n> any interest in these kinds of patches? If not, I will stop now!\n> (I have a 2-3 year old branch that addressed the '-Wsign-compare'\n> warnings, but that is probably beyond salvaging by now :( ).\n>\n> This series is available from: git://repo.or.cz/git/raj.git with the\n> branch name 'warn-master'. A trial merge to current 'next' and 'pu'\n> branches can be found at 'warn-next' and 'warn-pu' branches. (The\n> merge to 'next' went without problem, and 'pu' only required a fixup\n> to the builtin/commit patch).\n> [...]\n> What do you think?\n\nI think just send it to the list. We've seen worse, and it's easier to\nreview than needing to get out of the normal E-Mail workflow.\n\nI'm keen on getting us to stricter compiler warnings in general, but\nwe'll see whether this specific thing seems worth it in review.\n"},{"id":"376209","messageId":"fa9f98b0-5f8f-1d0e-9a87-70fed69520c1@ramsayjones.plus.com","threadId":"51172","inReplyTo":"87a7fbfptk.fsf@evledraar.gmail.com","subject":"Re: [PATCH 00/52] fix some -Wmissing-field-initializer warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-05-24T23:08:58Z","receivedAt":"2019-05-24T23:09:08Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 24/05/2019 23:30, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, May 24 2019, Ramsay Jones wrote:\n> \n>> [No, I won't be sending 52 patches to the list!]\n>> [...]\n>> This series does not fix any problems or add any new features, so it\n>> is not important (hence the tendency to 'slip'). I don't want to\n>> flood the mailing list with patches that nobody wants, so: is there\n>> any interest in these kinds of patches? If not, I will stop now!\n>> (I have a 2-3 year old branch that addressed the '-Wsign-compare'\n>> warnings, but that is probably beyond salvaging by now :( ).\n>>\n>> This series is available from: git://repo.or.cz/git/raj.git with the\n>> branch name 'warn-master'. A trial merge to current 'next' and 'pu'\n>> branches can be found at 'warn-next' and 'warn-pu' branches. (The\n>> merge to 'next' went without problem, and 'pu' only required a fixup\n>> to the builtin/commit patch).\n>> [...]\n>> What do you think?\n> \n> I think just send it to the list. We've seen worse, and it's easier to\n> review than needing to get out of the normal E-Mail workflow.\n\nI forgot to mention that this series has a long tail. The first\nten patches (in addition to all pedantic warnings) removes 1246\nwarnings. The next 10 removes another 70 and the final 32 patches\nonly removes 55 warnings (the last 18 patches remove only one\nwarning each).\n\nHmm, so maybe I should only send the first few out? \n\nI will give it some thought (while waiting for some more comments).\n\nThanks.\n\nATB,\nRamsay Jones\n\n\n"},{"id":"376434","messageId":"0f1c5a49-f971-848d-700e-9c124ae8e617@kdbg.org","threadId":"51172","inReplyTo":"cf0b720e-ad29-79e7-17d5-b69d50e7007e@ramsayjones.plus.com","subject":"Re: [PATCH 00/52] fix some -Wmissing-field-initializer warnings","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-05-30T08:47:37Z","receivedAt":"2019-05-30T08:47:42Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Hi Ramsay,\n\nI had a brief look at the series. IMO, it is a mistake to appease\n-Wmissing-field-initializer.\n\nWe have two sorts of initializers:\n\n - zero initializers: they just want to null out every field,\n   like CHILD_PROCESS_INIT and ad-hoc initializers of structs\n   such as xpparam_t pp = { 0 }; in range-diff.c\n\n - value initializers are always macros, such as STRING_LIST_INIT_DUP\n   and the OPT_* family.\n\nI am strongly against forcing zero initializers to write down a value\nfor every field. It is much more preferable to depend on that the\ncompiler does the right thing with them. -Wmissing-field-initializer\nwould provide guidance in the wrong direction. A zero initializer looks\nlike this: = { 0 }; and nothing else.\n\n(And for this reason, I also think it is wrong to change 0 to NULL in\ninitializers to appease sparse's \"zero used as pointer value\" warning.\nLet the compiler do the right thing.)\n\nValue initializers are a different matter. The changes you have prepared\naround struct option initializers are good, IMO. Whether or not you add\ntrailing zeros to the macros makes no difference for the users of the\nmacros.\n\n-- Hannes\n"},{"id":"376438","messageId":"20190530120441.GD31607@sigill.intra.peff.net","threadId":"51172","inReplyTo":"0f1c5a49-f971-848d-700e-9c124ae8e617@kdbg.org","subject":"Re: [PATCH 00/52] fix some -Wmissing-field-initializer warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-30T12:04:41Z","receivedAt":"2019-05-30T12:04:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 30, 2019 at 10:47:37AM +0200, Johannes Sixt wrote:\n\n> I had a brief look at the series. IMO, it is a mistake to appease\n> -Wmissing-field-initializer.\n> \n> We have two sorts of initializers:\n> \n>  - zero initializers: they just want to null out every field,\n>    like CHILD_PROCESS_INIT and ad-hoc initializers of structs\n>    such as xpparam_t pp = { 0 }; in range-diff.c\n> \n>  - value initializers are always macros, such as STRING_LIST_INIT_DUP\n>    and the OPT_* family.\n> \n> I am strongly against forcing zero initializers to write down a value\n> for every field. It is much more preferable to depend on that the\n> compiler does the right thing with them. -Wmissing-field-initializer\n> would provide guidance in the wrong direction. A zero initializer looks\n> like this: = { 0 }; and nothing else.\n\nI had a similar impression while perusing the commits. I don't mind\nforcing some extra work on programmers to appease a warning if\ndisregarding it is a common source of errors. But I didn't see any real\nbug-fixes in the series, so it doesn't seem like that good a tradeoff to\nme.\n\nContrast that with the -Wunused-parameters warning. I found a dozen or\nso actual bugs by sifting through the results, and another couple dozen\nspots where the code could be cleaned up or simplified. If we want to\nshut up the warning completely (so we can pay attention to it), we'll\nthen have to annotate probably a couple hundred spots, and keep those\nannotations up to date. But I feel better doing that knowing that it's\nshown real-world value.\n\n-Peff\n"},{"id":"376442","messageId":"dce88509-f4d5-1e7b-26f1-eff5cdccf1d0@ramsayjones.plus.com","threadId":"51172","inReplyTo":"20190530120441.GD31607@sigill.intra.peff.net","subject":"Re: [PATCH 00/52] fix some -Wmissing-field-initializer warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-05-30T14:59:01Z","receivedAt":"2019-05-30T14:59:13Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 30/05/2019 13:04, Jeff King wrote:\n> On Thu, May 30, 2019 at 10:47:37AM +0200, Johannes Sixt wrote:\n> \n>> I had a brief look at the series. IMO, it is a mistake to appease\n>> -Wmissing-field-initializer.\n>>\n>> We have two sorts of initializers:\n>>\n>>  - zero initializers: they just want to null out every field,\n>>    like CHILD_PROCESS_INIT and ad-hoc initializers of structs\n>>    such as xpparam_t pp = { 0 }; in range-diff.c\n>>\n>>  - value initializers are always macros, such as STRING_LIST_INIT_DUP\n>>    and the OPT_* family.\n>>\n>> I am strongly against forcing zero initializers to write down a value\n>> for every field. It is much more preferable to depend on that the\n>> compiler does the right thing with them. -Wmissing-field-initializer\n>> would provide guidance in the wrong direction. A zero initializer looks\n>> like this: = { 0 }; and nothing else.\n> \n> I had a similar impression while perusing the commits. I don't mind\n> forcing some extra work on programmers to appease a warning if\n> disregarding it is a common source of errors. But I didn't see any real\n> bug-fixes in the series, so it doesn't seem like that good a tradeoff to\n> me.\n> \n> Contrast that with the -Wunused-parameters warning. I found a dozen or\n> so actual bugs by sifting through the results, and another couple dozen\n> spots where the code could be cleaned up or simplified. If we want to\n> shut up the warning completely (so we can pay attention to it), we'll\n> then have to annotate probably a couple hundred spots, and keep those\n> annotations up to date. But I feel better doing that knowing that it's\n> shown real-world value.\n\nOK, I will drop this branch then.\n\nThanks all.\n\nATB,\nRamsay Jones\n\n\n"}]}