{"thread":{"id":"57227","subject":"[PATCH 0/3] Fix SunCC compiler complaints new in v2.35.0-rc0","startedAt":"2022-01-11T16:40:43Z","lastAt":"2022-01-13T21:38:59Z","messageCount":32,"participants":["Ævar Arnfjörð Bjarmason","Han-Wen Nienhuys","René Scharfe","Taylor Blau","Johannes Sixt","Emily Shaffer","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"445926","messageId":"cover-0.3-00000000000-20220111T163908Z-avarab@gmail.com","threadId":"57227","inReplyTo":null,"subject":"[PATCH 0/3] Fix SunCC compiler complaints new in v2.35.0-rc0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-11T16:40:20Z","receivedAt":"2022-01-11T16:40:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This trivial set of patches fixes compiler complaints from SunCC on\nSolaris that are new in v2.35.0-rc0.\n\nThe first two are only a minor annoyance, and not the first or only\nwarnings of those categories that we emit, but if we can avoid adding\nnew ones...\n\nThe third one is a generic integer overflow bug, and will probably\nresult in logic errors or failures in the reftable tests on some other\nplatform.\n\nÆvar Arnfjörð Bjarmason (3):\n test-tool genzeros: initialize \"zeros\" to avoid SunCC warning\n reftable: remove unreachable \"return\" statements\n reftable tests: avoid \"int\" overflow, use \"uint64_t\"\n\n reftable/merged_test.c | 4 ++--\n reftable/refname.c | 1 -\n reftable/writer.c | 1 -\n t/helper/test-genzeros.c | 3 +--\n 4 files changed, 3 insertions(+), 6 deletions(-)\n\n-- \n2.35.0.rc0.844.gb5945183dcf\n\n"},{"id":"445927","messageId":"patch-1.3-4dadf7320ab-20220111T163908Z-avarab@gmail.com","threadId":"57227","inReplyTo":"cover-0.3-00000000000-20220111T163908Z-avarab@gmail.com","subject":"[PATCH 1/3] test-tool genzeros: initialize \"zeros\" to avoid SunCC warning","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-11T16:40:21Z","receivedAt":"2022-01-11T16:40:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"It isn't important for optimization to have this be \"static\", so let's\njust initialize it and avoid this warning on Sun Studio 12.5:\n\n \"t/helper/test-genzeros.c\", line 7: warning: const object should have initializer: zeros\n\nThis amends code added in df7000cd910 (test-tool genzeros: generate\nlarge amounts of data more efficiently, 2021-11-02), and first tagged\nwith v2.35.0-rc0.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/helper/test-genzeros.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\nindex 8ca988d6216..5dc89eda0cb 100644\n--- a/t/helper/test-genzeros.c\n+++ b/t/helper/test-genzeros.c\n@@ -3,8 +3,7 @@\n \n int cmd__genzeros(int argc, const char **argv)\n {\n-\t/* static, so that it is NUL-initialized */\n-\tstatic const char zeros[256 * 1024];\n+\tconst char zeros[256 * 1024] = { 0 };\n \tintmax_t count;\n \tssize_t n;\n \n-- \n2.35.0.rc0.844.gb5945183dcf\n\n"},{"id":"445928","messageId":"patch-3.3-93112305523-20220111T163908Z-avarab@gmail.com","threadId":"57227","inReplyTo":"cover-0.3-00000000000-20220111T163908Z-avarab@gmail.com","subject":"[PATCH 3/3] reftable tests: avoid \"int\" overflow, use \"uint64_t\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-11T16:40:23Z","receivedAt":"2022-01-11T16:40:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change code added in 1ae2b8cda84 (reftable: add merged table view,\n2021-10-07) to consistently use the \"uint64_t\" type. These \"min\" and\n\"max\" variables get passed in the body of this function to a function\nwhose prototype is:\n\n [...] reftable_writer_set_limits([...], uint64_t min, uint64_t max\n\nThis avoids the following warning on SunCC 12.5 on\ngcc211.fsffrance.org:\n\n \"reftable/merged_test.c\", line 27: warning: initializer does not fit or is out of range: 0xffffffff\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n reftable/merged_test.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/merged_test.c b/reftable/merged_test.c\nindex 24461e8a802..b87ff495dfd 100644\n--- a/reftable/merged_test.c\n+++ b/reftable/merged_test.c\n@@ -24,8 +24,8 @@ license that can be found in the LICENSE file or at\n static void write_test_table(struct strbuf *buf,\n \t\t\t struct reftable_ref_record refs[], int n)\n {\n-\tint min = 0xffffffff;\n-\tint max = 0;\n+\tuint64_t min = 0xffffffff;\n+\tuint64_t max = 0;\n \tint i = 0;\n \tint err;\n \n-- \n2.35.0.rc0.844.gb5945183dcf\n\n"},{"id":"445929","messageId":"patch-2.3-7a6a69314b5-20220111T163908Z-avarab@gmail.com","threadId":"57227","inReplyTo":"cover-0.3-00000000000-20220111T163908Z-avarab@gmail.com","subject":"[PATCH 2/3] reftable: remove unreachable \"return\" statements","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-11T16:40:22Z","receivedAt":"2022-01-11T16:40:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove unreachable return statements added in acb533440fc (reftable:\nimplement refname validation, 2021-10-07) and f14bd719349 (reftable:\nwrite reftable files, 2021-10-07).\n\nThis avoids the following warnings on SunCC 12.5 on\ngcc211.fsffrance.org:\n\n \"reftable/refname.c\", line 135: warning: statement not reached\n \"reftable/refname.c\", line 135: warning: statement not reached\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n reftable/refname.c | 1 -\n reftable/writer.c | 1 -\n 2 files changed, 2 deletions(-)\n\ndiff --git a/reftable/refname.c b/reftable/refname.c\nindex 95734969324..136001bc2c7 100644\n--- a/reftable/refname.c\n+++ b/reftable/refname.c\n@@ -132,7 +132,6 @@ static int validate_refname(const char *name)\n \t\t\treturn REFTABLE_REFNAME_ERROR;\n \t\tname = next + 1;\n \t}\n-\treturn 0;\n }\n \n int validate_ref_record_addition(struct reftable_table tab,\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 35c8649c9b7..70a7bf142a2 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -39,7 +39,6 @@ writer_reftable_block_stats(struct reftable_writer *w, uint8_t typ)\n \t\treturn &w->stats.log_stats;\n \t}\n \tabort();\n-\treturn NULL;\n }\n \n /* write data, queuing the padding for the next write. Returns negative for\n-- \n2.35.0.rc0.844.gb5945183dcf\n\n"},{"id":"445932","messageId":"CAFQ2z_M1skVCFYQ-kM5dMVXcUxV-uR+Zf89dKxgc2v1c1tre=A@mail.gmail.com","threadId":"57227","inReplyTo":"cover-0.3-00000000000-20220111T163908Z-avarab@gmail.com","subject":"Re: [PATCH 0/3] Fix SunCC compiler complaints new in v2.35.0-rc0","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2022-01-11T17:06:28Z","receivedAt":"2022-01-11T17:06:44Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Tue, Jan 11, 2022 at 5:40 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> This trivial set of patches fixes compiler complaints from SunCC on\n> Solaris that are new in v2.35.0-rc0.\n\nThanks, looks fine in principle.\n\nFor the switch, maybe the block types should be an enum. Is there\ntooling that would detect a missing case in that case, so we could\ndrop the abort() ?\n\nFor the 0xffffffff constant, there is probably a better symbolic\nconstant somewhere. Or could we say (1<<64) -1 without causing\noverflow?\n\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"445950","messageId":"c3228d53-5997-d86c-87a5-5861dacde518@web.de","threadId":"57227","inReplyTo":"CAFQ2z_M1skVCFYQ-kM5dMVXcUxV-uR+Zf89dKxgc2v1c1tre=A@mail.gmail.com","subject":"Re: [PATCH 0/3] Fix SunCC compiler complaints new in v2.35.0-rc0","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-01-11T18:36:09Z","receivedAt":"2022-01-11T18:36:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.01.22 um 18:06 schrieb Han-Wen Nienhuys:\n> On Tue, Jan 11, 2022 at 5:40 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> This trivial set of patches fixes compiler complaints from SunCC on\n>> Solaris that are new in v2.35.0-rc0.\n>\n> Thanks, looks fine in principle.\n>\n> For the switch, maybe the block types should be an enum. Is there\n> tooling that would detect a missing case in that case, so we could\n> drop the abort() ?\n>\n> For the 0xffffffff constant, there is probably a better symbolic\n> constant somewhere. Or could we say (1<<64) -1 without causing\n> overflow?\n\nHow about -1?\n\nOr you could initialize min and max to the actual value of the first\nitem instead of to the absolute limits of the data type.\n\nRené\n"},{"id":"445952","messageId":"Yd3VKjvb7AR0/wI9@nand.local","threadId":"57227","inReplyTo":"patch-1.3-4dadf7320ab-20220111T163908Z-avarab@gmail.com","subject":"Re: [PATCH 1/3] test-tool genzeros: initialize \"zeros\" to avoid SunCC warning","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-11T19:06:18Z","receivedAt":"2022-01-11T19:06:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 11, 2022 at 05:40:21PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> It isn't important for optimization to have this be \"static\", so let's\n> just initialize it and avoid this warning on Sun Studio 12.5:\n>\n> \"t/helper/test-genzeros.c\", line 7: warning: const object should have initializer: zeros\n\nI agree that whether or not this variable is declared statically does\nnot matter for our purposes, since we call cmd__genzeros() exactly once\nper invocation of the test helper. So we're never paying the cost to\nre-declare a variable on the stack since that stack frame only gets\ncreated once.\n\nAnd I don't care for the purposes of a reroll on such a trivial\nquestion, but I do think that this patch leaves something out that was\nincluded in the cover letter. Namely, that this is not the first such\nwarning of this kind from SunCC. Or in other words, that we have lots of\nstatic const objects that we depend on being zero'd (and which we can\nsafely assume _are_ zerod, since they are declared statically), but that\neach of them already generates a warning from SunCC.\n\nMentioning that would give readers an impression of why this was the\nonly spot touched instead of every static const variable.\n\nBut if you take that line of thinking further, I have a hard time\nimagining that it's worth fixing small issues like this in piecemeal\nwhen there are already many such warnings. We _could_ fix all of them at\nonce and amend Documentation/CodingGuidelines, but that seems like a lot\nof work for an issue that does not seem to be causing much pain at all.\n\n> diff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\n> index 8ca988d6216..5dc89eda0cb 100644\n> --- a/t/helper/test-genzeros.c\n> +++ b/t/helper/test-genzeros.c\n> @@ -3,8 +3,7 @@\n>\n> int cmd__genzeros(int argc, const char **argv)\n> {\n> -\t/* static, so that it is NUL-initialized */\n> -\tstatic const char zeros[256 * 1024];\n> +\tconst char zeros[256 * 1024] = { 0 };\n\nSo this transformation is just fine, but I imagine that we probably\ncould have just as easily left it alone.\n\nThanks,\nTaylor\n"},{"id":"445954","messageId":"Yd3XpLaZ3qc25PzQ@nand.local","threadId":"57227","inReplyTo":"patch-2.3-7a6a69314b5-20220111T163908Z-avarab@gmail.com","subject":"Re: [PATCH 2/3] reftable: remove unreachable \"return\" statements","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-11T19:16:52Z","receivedAt":"2022-01-11T19:18:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 11, 2022 at 05:40:22PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> Remove unreachable return statements added in acb533440fc (reftable:\n> implement refname validation, 2021-10-07) and f14bd719349 (reftable:\n> write reftable files, 2021-10-07).\n>\n> This avoids the following warnings on SunCC 12.5 on\n> gcc211.fsffrance.org:\n>\n> \"reftable/refname.c\", line 135: warning: statement not reached\n> \"reftable/refname.c\", line 135: warning: statement not reached\n\nInteresting. From a cursory reading, I agree with the assessment of\nat least my compiler that these return statements are both unnecessary,\nbut...\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> reftable/refname.c | 1 -\n> reftable/writer.c | 1 -\n> 2 files changed, 2 deletions(-)\n>\n> diff --git a/reftable/refname.c b/reftable/refname.c\n> index 95734969324..136001bc2c7 100644\n> --- a/reftable/refname.c\n> +++ b/reftable/refname.c\n> @@ -132,7 +132,6 @@ static int validate_refname(const char *name)\n> \t\t\treturn REFTABLE_REFNAME_ERROR;\n> \t\tname = next + 1;\n> \t}\n> -\treturn 0;\n> }\n\nIn this case the loop inside of validate_refname() should always\nterminate the function within the loop body. But removing this return\nstatement here relies on the compiler to determine that fact.\n\nI could well imagine on the other end of the spectrum there exists a\ncompiler which _doesn't_ make this inference pass, and would complain\nabout the opposite thing as you're reporting from SunCC (i.e., that this\nfunction which returns something other than void does not have a return\nstatement outside of the loop).\n\nSo in that sense, I disagree with the guidance of SunCC's warning. In\nother words: by quelching this warning under one compiler, are we\nintroducing a new warning under a different/less advanced compiler?\n\n> int validate_ref_record_addition(struct reftable_table tab,\n> diff --git a/reftable/writer.c b/reftable/writer.c\n> index 35c8649c9b7..70a7bf142a2 100644\n> --- a/reftable/writer.c\n> +++ b/reftable/writer.c\n> @@ -39,7 +39,6 @@ writer_reftable_block_stats(struct reftable_writer *w, uint8_t typ)\n> \t\treturn &w->stats.log_stats;\n> \t}\n> \tabort();\n> -\treturn NULL;\n> }\n\nHere I'm less skeptical, since it's almost certain that any compiler\nwould recognize this call to abort() as terminating the whole program.\nSo it should be able to infer that anything after it is unreachable.\n\nBut even though I'm less skeptical, I'm not sure that I would make the\nsame bet (though in practice this one is probably fine since there are\nlikely plenty of functions which end in the more standard `die()` and do\nnot have a return path).\n\nCan reftable call die()? Or is this the least-common denominator among\nGit and libgit2 for terminating a running program?\n\nThanks,\nTaylor\n"},{"id":"445956","messageId":"Yd3aZ5Q9Ph0H+Ijn@nand.local","threadId":"57227","inReplyTo":"patch-3.3-93112305523-20220111T163908Z-avarab@gmail.com","subject":"Re: [PATCH 3/3] reftable tests: avoid \"int\" overflow, use \"uint64_t\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-11T19:28:39Z","receivedAt":"2022-01-11T19:28:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 11, 2022 at 05:40:23PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> diff --git a/reftable/merged_test.c b/reftable/merged_test.c\n> index 24461e8a802..b87ff495dfd 100644\n> --- a/reftable/merged_test.c\n> +++ b/reftable/merged_test.c\n> @@ -24,8 +24,8 @@ license that can be found in the LICENSE file or at\n> static void write_test_table(struct strbuf *buf,\n> \t\t\t struct reftable_ref_record refs[], int n)\n> {\n> -\tint min = 0xffffffff;\n> -\tint max = 0;\n> +\tuint64_t min = 0xffffffff;\n> +\tuint64_t max = 0;\n\nHan-Wen: it looks like the loop below the context here is to set the\nmin/max of update_index over all of the ref records?\n\nIf so, making these comparisons all unsigned makes sense to me. In\npractice it's probably fine at least from a signedness perspective,\nsince the compiler _should_ be coercing both operands to unsigned.\n\nBut perhaps not so from a width perspective, if sizeof(int) != 8 (though\nI suspect in practice that we are unlikely to have enough possible\nvalues of update_index for that to matter).\n\nIn any case, you're only setting the lower half of `min` high. Maybe:\n\n uint64_t min = ~0ul;\n\ninstead?\n\nThanks,\nTaylor\n"},{"id":"445957","messageId":"CAFQ2z_M0Fjjx8tAd_cXqk9K9q43hwienEy=UsLJi2aW8G3sEwA@mail.gmail.com","threadId":"57227","inReplyTo":"Yd3aZ5Q9Ph0H+Ijn@nand.local","subject":"Re: [PATCH 3/3] reftable tests: avoid \"int\" overflow, use \"uint64_t\"","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2022-01-11T19:31:47Z","receivedAt":"2022-01-11T19:32:03Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau wrote:\n>\n> On Tue, Jan 11, 2022 at 05:40:23PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > diff --git a/reftable/merged_test.c b/reftable/merged_test.c\n> > index 24461e8a802..b87ff495dfd 100644\n> > --- a/reftable/merged_test.c\n> > +++ b/reftable/merged_test.c\n> > @@ -24,8 +24,8 @@ license that can be found in the LICENSE file or at\n> > static void write_test_table(struct strbuf *buf,\n> > struct reftable_ref_record refs[], int n)\n> > {\n> > - int min = 0xffffffff;\n> > - int max = 0;\n> > + uint64_t min = 0xffffffff;\n> > + uint64_t max = 0;\n>\n> Han-Wen: it looks like the loop below the context here is to set the\n> min/max of update_index over all of the ref records?\n\ncorrect.\n\n> But perhaps not so from a width perspective, if sizeof(int) != 8 (though\n> I suspect in practice that we are unlikely to have enough possible\n> values of update_index for that to matter).\n\ncorrect.\n\n> In any case, you're only setting the lower half of `min` high. Maybe:\n>\n> uint64_t min = ~0ul;\n\nyeah, that works.\n\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"445959","messageId":"Yd3dao5Qh5yl/cBs@nand.local","threadId":"57227","inReplyTo":"CAFQ2z_M0Fjjx8tAd_cXqk9K9q43hwienEy=UsLJi2aW8G3sEwA@mail.gmail.com","subject":"Re: [PATCH 3/3] reftable tests: avoid \"int\" overflow, use \"uint64_t\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-11T19:41:30Z","receivedAt":"2022-01-11T19:41:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:\n> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau wrote:\n> > In any case, you're only setting the lower half of `min` high. Maybe:\n> >\n> > uint64_t min = ~0ul;\n>\n> yeah, that works.\n\nI'm pretty sure this is OK on 32-bit systems, too, but confirmation from\nsomebody more confident than I in this area would be welcome :).\n\nThanks,\nTaylor\n"},{"id":"445963","messageId":"45baffd7-c9f3-cc52-47b4-ea0fee0182a8@kdbg.org","threadId":"57227","inReplyTo":"Yd3dao5Qh5yl/cBs@nand.local","subject":"Re: [PATCH 3/3] reftable tests: avoid \"int\" overflow, use \"uint64_t\"","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2022-01-11T20:08:46Z","receivedAt":"2022-01-11T20:08:53Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.01.22 um 20:41 schrieb Taylor Blau:\n> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:\n>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau wrote:\n>>> In any case, you're only setting the lower half of `min` high. Maybe:\n>>>\n>>> uint64_t min = ~0ul;\n>>\n>> yeah, that works.\n> \n> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from\n> somebody more confident than I in this area would be welcome :).\n\nIt does not work on Windows: unsigned long is 32 bits wide. You have to\nmake it\n\n uint64_t min = ~(uint64_t)0;\n\n-- Hannes\n"},{"id":"445966","messageId":"Yd3mCVLyxkVTNcS9@nand.local","threadId":"57227","inReplyTo":"45baffd7-c9f3-cc52-47b4-ea0fee0182a8@kdbg.org","subject":"Re: [PATCH 3/3] reftable tests: avoid \"int\" overflow, use \"uint64_t\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-11T20:18:17Z","receivedAt":"2022-01-11T20:18:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:\n> Am 11.01.22 um 20:41 schrieb Taylor Blau:\n> > On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:\n> >> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau wrote:\n> >>> In any case, you're only setting the lower half of `min` high. Maybe:\n> >>>\n> >>> uint64_t min = ~0ul;\n> >>\n> >> yeah, that works.\n> >\n> > I'm pretty sure this is OK on 32-bit systems, too, but confirmation from\n> > somebody more confident than I in this area would be welcome :).\n>\n> It does not work on Windows: unsigned long is 32 bits wide. You have to\n> make it\n>\n> uint64_t min = ~(uint64_t)0;\n\nPerfect; this is exactly what I was looking for. Thanks!\n\nTaylor\n"},{"id":"445967","messageId":"e8e2c37d-39f1-0ae4-7894-07e85628a757@kdbg.org","threadId":"57227","inReplyTo":"Yd3mCVLyxkVTNcS9@nand.local","subject":"Re: [PATCH 3/3] reftable tests: avoid \"int\" overflow, use \"uint64_t\"","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2022-01-11T20:21:11Z","receivedAt":"2022-01-11T20:21:16Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.01.22 um 21:18 schrieb Taylor Blau:\n> On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:\n>> Am 11.01.22 um 20:41 schrieb Taylor Blau:\n>>> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:\n>>>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau wrote:\n>>>>> In any case, you're only setting the lower half of `min` high. Maybe:\n>>>>>\n>>>>> uint64_t min = ~0ul;\n>>>>\n>>>> yeah, that works.\n>>>\n>>> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from\n>>> somebody more confident than I in this area would be welcome :).\n>>\n>> It does not work on Windows: unsigned long is 32 bits wide. You have to\n>> make it\n>>\n>> uint64_t min = ~(uint64_t)0;\n> \n> Perfect; this is exactly what I was looking for. Thanks!\n\nActually, on second thought, UINT64_MAX would be even better.\n\n-- Hannes\n"},{"id":"445968","messageId":"Yd3njKcopXVZ+69C@nand.local","threadId":"57227","inReplyTo":"e8e2c37d-39f1-0ae4-7894-07e85628a757@kdbg.org","subject":"Re: [PATCH 3/3] reftable tests: avoid \"int\" overflow, use \"uint64_t\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-11T20:24:44Z","receivedAt":"2022-01-11T20:24:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 11, 2022 at 09:21:11PM +0100, Johannes Sixt wrote:\n> Am 11.01.22 um 21:18 schrieb Taylor Blau:\n> > On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:\n> >> Am 11.01.22 um 20:41 schrieb Taylor Blau:\n> >>> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:\n> >>>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau wrote:\n> >>>>> In any case, you're only setting the lower half of `min` high. Maybe:\n> >>>>>\n> >>>>> uint64_t min = ~0ul;\n> >>>>\n> >>>> yeah, that works.\n> >>>\n> >>> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from\n> >>> somebody more confident than I in this area would be welcome :).\n> >>\n> >> It does not work on Windows: unsigned long is 32 bits wide. You have to\n> >> make it\n> >>\n> >> uint64_t min = ~(uint64_t)0;\n> >\n> > Perfect; this is exactly what I was looking for. Thanks!\n>\n> Actually, on second thought, UINT64_MAX would be even better.\n\n:-). I think that either is probably fine; I couldn't remember if\nUINT64_MAX was part of POSIX or not (and clearly didn't bother to check!)\n\nThanks,\nTaylor\n"},{"id":"445982","messageId":"CAJoAoZ=T6SJh=gp4BbEbw1F2VSnN_NBeh+dKiTihR7vewLh3zg@mail.gmail.com","threadId":"57227","inReplyTo":"cover-0.3-00000000000-20220111T163908Z-avarab@gmail.com","subject":"Re: [PATCH 0/3] Fix SunCC compiler complaints new in v2.35.0-rc0","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-01-12T01:22:44Z","receivedAt":"2022-01-12T01:22:59Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Jan 11, 2022 at 5:21 PM Emily Shaffer wrote:\n\nugh, sorry for the thumbfinger. Please ignore.\n"},{"id":"446001","messageId":"220112.865yqpxge2.gmgdl@evledraar.gmail.com","threadId":"57227","inReplyTo":"Yd3XpLaZ3qc25PzQ@nand.local","subject":"Re: [PATCH 2/3] reftable: remove unreachable \"return\" statements","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-12T12:47:40Z","receivedAt":"2022-01-12T12:55:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 11 2022, Taylor Blau wrote:\n\n> On Tue, Jan 11, 2022 at 05:40:22PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> Remove unreachable return statements added in acb533440fc (reftable:\n>> implement refname validation, 2021-10-07) and f14bd719349 (reftable:\n>> write reftable files, 2021-10-07).\n>>\n>> This avoids the following warnings on SunCC 12.5 on\n>> gcc211.fsffrance.org:\n>>\n>> \"reftable/refname.c\", line 135: warning: statement not reached\n>> \"reftable/refname.c\", line 135: warning: statement not reached\n>\n> Interesting. From a cursory reading, I agree with the assessment of\n> at least my compiler that these return statements are both unnecessary,\n> but...\n>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> reftable/refname.c | 1 -\n>> reftable/writer.c | 1 -\n>> 2 files changed, 2 deletions(-)\n>>\n>> diff --git a/reftable/refname.c b/reftable/refname.c\n>> index 95734969324..136001bc2c7 100644\n>> --- a/reftable/refname.c\n>> +++ b/reftable/refname.c\n>> @@ -132,7 +132,6 @@ static int validate_refname(const char *name)\n>> \t\t\treturn REFTABLE_REFNAME_ERROR;\n>> \t\tname = next + 1;\n>> \t}\n>> -\treturn 0;\n>> }\n>\n> In this case the loop inside of validate_refname() should always\n> terminate the function within the loop body. But removing this return\n> statement here relies on the compiler to determine that fact.\n>\n> I could well imagine on the other end of the spectrum there exists a\n> compiler which _doesn't_ make this inference pass, and would complain\n> about the opposite thing as you're reporting from SunCC (i.e., that this\n> function which returns something other than void does not have a return\n> statement outside of the loop).\n>\n> So in that sense, I disagree with the guidance of SunCC's warning. In\n> other words: by quelching this warning under one compiler, are we\n> introducing a new warning under a different/less advanced compiler?\n\nI'd think that any compiler who'd warn about this sort of thing at all\nwould be able to spot constructs like this one, which are basically:\n\n while (1) {\n \t...\n if (x)\n \treturn;\n\t...\n }\n return; /* unreachable */\n\nWhere the elided code contains no \"break\", \"goto\" or other mechanism for\nexiting the for-loop.\n\nI.e. GCC and Clang don't bother to note the unreachable code, but I\ndon't think the reverse will be true, that a compiler will say that a\n\"return\" is missing there. Having a function be just a loop body that\nreturns an some point is a common pattern.\n\n>> int validate_ref_record_addition(struct reftable_table tab,\n>> diff --git a/reftable/writer.c b/reftable/writer.c\n>> index 35c8649c9b7..70a7bf142a2 100644\n>> --- a/reftable/writer.c\n>> +++ b/reftable/writer.c\n>> @@ -39,7 +39,6 @@ writer_reftable_block_stats(struct reftable_writer *w, uint8_t typ)\n>> \t\treturn &w->stats.log_stats;\n>> \t}\n>> \tabort();\n>> -\treturn NULL;\n>> }\n>\n> Here I'm less skeptical, since it's almost certain that any compiler\n> would recognize this call to abort() as terminating the whole program.\n> So it should be able to infer that anything after it is unreachable.\n\nThat's also correct, but in terms of compiler implementations I'd think\nyou'd get basic loop flow analysis first, and the annotation of\nunreturn-able functions like abort() or a custom die() later.\n> ...\n"},{"id":"446026","messageId":"nycvar.QRO.7.76.6.2201121515490.2121@tvgsbejvaqbjf.bet","threadId":"57227","inReplyTo":"Yd3njKcopXVZ+69C@nand.local","subject":"Re: [PATCH 3/3] reftable tests: avoid \"int\" overflow, use \"uint64_t\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-12T14:18:20Z","receivedAt":"2022-01-12T14:18:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Taylor,\n\nOn Tue, 11 Jan 2022, Taylor Blau wrote:\n\n> On Tue, Jan 11, 2022 at 09:21:11PM +0100, Johannes Sixt wrote:\n> > Am 11.01.22 um 21:18 schrieb Taylor Blau:\n> > > On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:\n> > >> Am 11.01.22 um 20:41 schrieb Taylor Blau:\n> > >>> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:\n> > >>>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau wrote:\n> > >>>>> In any case, you're only setting the lower half of `min` high. Maybe:\n> > >>>>>\n> > >>>>> uint64_t min = ~0ul;\n> > >>>>\n> > >>>> yeah, that works.\n> > >>>\n> > >>> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from\n> > >>> somebody more confident than I in this area would be welcome :).\n> > >>\n> > >> It does not work on Windows: unsigned long is 32 bits wide. You have to\n> > >> make it\n> > >>\n> > >> uint64_t min = ~(uint64_t)0;\n> > >\n> > > Perfect; this is exactly what I was looking for. Thanks!\n> >\n> > Actually, on second thought, UINT64_MAX would be even better.\n>\n> :-). I think that either is probably fine; I couldn't remember if\n> UINT64_MAX was part of POSIX or not (and clearly didn't bother to check!)\n\nThe best solution, of course, would be to `git grep` through the code and\nsee that UINT64_MAX is not used at all.\n\nAnd that brings us to the question whether we really need to ensure that\nexactly, precisely 64 bit are used for this variable? The answer is: no.\nWe may need it to be larger than 32-bit, so why not go for `uintmax_t` and\n`UINTMAX_MAX`, both of which _are_ already used in Git's source code?\n\nCiao,\nDscho\n"},{"id":"446027","messageId":"nycvar.QRO.7.76.6.2201121518360.2121@tvgsbejvaqbjf.bet","threadId":"57227","inReplyTo":"patch-1.3-4dadf7320ab-20220111T163908Z-avarab@gmail.com","subject":"Re: [PATCH 1/3] test-tool genzeros: initialize \"zeros\" to avoid SunCC warning","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-12T14:21:46Z","receivedAt":"2022-01-12T14:21:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Tue, 11 Jan 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> It isn't important for optimization to have this be \"static\", so let's\n> just initialize it and avoid this warning on Sun Studio 12.5:\n>\n> \"t/helper/test-genzeros.c\", line 7: warning: const object should have initializer: zeros\n>\n> This amends code added in df7000cd910 (test-tool genzeros: generate\n> large amounts of data more efficiently, 2021-11-02), and first tagged\n> with v2.35.0-rc0.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/helper/test-genzeros.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\n> index 8ca988d6216..5dc89eda0cb 100644\n> --- a/t/helper/test-genzeros.c\n> +++ b/t/helper/test-genzeros.c\n> @@ -3,8 +3,7 @@\n>\n> int cmd__genzeros(int argc, const char **argv)\n> {\n> -\t/* static, so that it is NUL-initialized */\n> -\tstatic const char zeros[256 * 1024];\n> +\tconst char zeros[256 * 1024] = { 0 };\n\nThis diff does two things: add an initializer, and turn the variable into\na `static`. The former is the actual fix that is required. The latter is\nnot. During the -rc phase, we do not want to see any of the latter. It is\nunnecessarily controversial and distracting, and can easily be postponed\nuntil January 25th, 2022.\n\nCiao,\nJohannes\n\n> \tintmax_t count;\n> \tssize_t n;\n>\n> --\n> 2.35.0.rc0.844.gb5945183dcf\n>\n>\n"},{"id":"446049","messageId":"xmqqh7a8vkuq.fsf@gitster.g","threadId":"57227","inReplyTo":"e8e2c37d-39f1-0ae4-7894-07e85628a757@kdbg.org","subject":"Re: [PATCH 3/3] reftable tests: avoid \"int\" overflow, use \"uint64_t\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-12T19:02:05Z","receivedAt":"2022-01-12T19:02:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 11.01.22 um 21:18 schrieb Taylor Blau:\n>> On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:\n>>> Am 11.01.22 um 20:41 schrieb Taylor Blau:\n>>>> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:\n>>>>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau wrote:\n>>>>>> In any case, you're only setting the lower half of `min` high. Maybe:\n>>>>>>\n>>>>>> uint64_t min = ~0ul;\n>>>>>\n>>>>> yeah, that works.\n>>>>\n>>>> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from\n>>>> somebody more confident than I in this area would be welcome :).\n>>>\n>>> It does not work on Windows: unsigned long is 32 bits wide. You have to\n>>> make it\n>>>\n>>> uint64_t min = ~(uint64_t)0;\n>> \n>> Perfect; this is exactly what I was looking for. Thanks!\n\nThat sounds perfect.\n\n> Actually, on second thought, UINT64_MAX would be even better.\n\nI wouldn't introduce use of UINT64_MAX, which \"git grep\" does not\nproduce any hits for.\n\nUnless it is very early in a development cycle, that is, in which\ncase we have enough time to help platforms that are not quite POSIX.\n\n"},{"id":"446050","messageId":"Yd8m2YeDZrgSiFzJ@nand.local","threadId":"57227","inReplyTo":"xmqqh7a8vkuq.fsf@gitster.g","subject":"Re: [PATCH 3/3] reftable tests: avoid \"int\" overflow, use \"uint64_t\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-12T19:07:05Z","receivedAt":"2022-01-12T19:07:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 12, 2022 at 11:02:05AM -0800, Junio C Hamano wrote:\n> Johannes Sixt writes:\n>\n> > Am 11.01.22 um 21:18 schrieb Taylor Blau:\n> >> On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:\n> >>> Am 11.01.22 um 20:41 schrieb Taylor Blau:\n> >>>> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:\n> >>>>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau wrote:\n> >>>>>> In any case, you're only setting the lower half of `min` high. Maybe:\n> >>>>>>\n> >>>>>> uint64_t min = ~0ul;\n> >>>>>\n> >>>>> yeah, that works.\n> >>>>\n> >>>> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from\n> >>>> somebody more confident than I in this area would be welcome :).\n> >>>\n> >>> It does not work on Windows: unsigned long is 32 bits wide. You have to\n> >>> make it\n> >>>\n> >>> uint64_t min = ~(uint64_t)0;\n> >>\n> >> Perfect; this is exactly what I was looking for. Thanks!\n>\n> That sounds perfect.\n>\n> > Actually, on second thought, UINT64_MAX would be even better.\n>\n> I wouldn't introduce use of UINT64_MAX, which \"git grep\" does not\n> produce any hits for.\n\n> Unless it is very early in a development cycle, that is, in which\n> case we have enough time to help platforms that are not quite POSIX.\n\nYep, I agree that avoiding introducing the first instance of UINT64_MAX\nin our tree is worth doing (probably in general, but certainly now that\nwe're past even -rc0).\n\nEither `~(uint64_t)0` or `UINTMAX_MAX` would be fine with me.\n\nThanks,\nTaylor\n"},{"id":"446051","messageId":"Yd8njHz2m099iDfL@nand.local","threadId":"57227","inReplyTo":"nycvar.QRO.7.76.6.2201121518360.2121@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/3] test-tool genzeros: initialize \"zeros\" to avoid SunCC warning","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-12T19:10:04Z","receivedAt":"2022-01-12T19:10:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 12, 2022 at 03:21:46PM +0100, Johannes Schindelin wrote:\n> > diff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\n> > index 8ca988d6216..5dc89eda0cb 100644\n> > --- a/t/helper/test-genzeros.c\n> > +++ b/t/helper/test-genzeros.c\n> > @@ -3,8 +3,7 @@\n> >\n> > int cmd__genzeros(int argc, const char **argv)\n> > {\n> > -\t/* static, so that it is NUL-initialized */\n> > -\tstatic const char zeros[256 * 1024];\n> > +\tconst char zeros[256 * 1024] = { 0 };\n>\n> This diff does two things: add an initializer, and turn the variable into\n> a `static`. The former is the actual fix that is required. The latter is\n> not. During the -rc phase, we do not want to see any of the latter. It is\n> unnecessarily controversial and distracting, and can easily be postponed\n> until January 25th, 2022.\n\nThis assumes that making the declaration non-static isn't necessary to\nfix the warning from SunCC.\n\nI would guess that in reality it probably isn't, so removing the static\ndesignation is a stray change, and this would have been easier to grok\nas simply:\n\n -\tstatic const char zeros[256 * 1024];\n +\tstatic const char zeros[256 * 1024] = { 0 };\n\nBut to be honest I don't think it is _that_ big of a deal to make such a\nsmall change during this point of the development cycle.\n\nThanks,\nTaylor\n"},{"id":"446054","messageId":"Yd8pzOh1HLvDWSPg@nand.local","threadId":"57227","inReplyTo":"220112.865yqpxge2.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/3] reftable: remove unreachable \"return\" statements","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-12T19:19:40Z","receivedAt":"2022-01-12T19:20:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 12, 2022 at 01:47:40PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> >> Signed-off-by: Ævar Arnfjörð Bjarmason \n> >> ---\n> >> reftable/refname.c | 1 -\n> >> reftable/writer.c | 1 -\n> >> 2 files changed, 2 deletions(-)\n> >>\n> >> diff --git a/reftable/refname.c b/reftable/refname.c\n> >> index 95734969324..136001bc2c7 100644\n> >> --- a/reftable/refname.c\n> >> +++ b/reftable/refname.c\n> >> @@ -132,7 +132,6 @@ static int validate_refname(const char *name)\n> >> \t\t\treturn REFTABLE_REFNAME_ERROR;\n> >> \t\tname = next + 1;\n> >> \t}\n> >> -\treturn 0;\n> >> }\n> >\n> > In this case the loop inside of validate_refname() should always\n> > terminate the function within the loop body. But removing this return\n> > statement here relies on the compiler to determine that fact.\n> >\n> > I could well imagine on the other end of the spectrum there exists a\n> > compiler which _doesn't_ make this inference pass, and would complain\n> > about the opposite thing as you're reporting from SunCC (i.e., that this\n> > function which returns something other than void does not have a return\n> > statement outside of the loop).\n> >\n> > So in that sense, I disagree with the guidance of SunCC's warning. In\n> > other words: by quelching this warning under one compiler, are we\n> > introducing a new warning under a different/less advanced compiler?\n>\n> I'd think that any compiler who'd warn about this sort of thing at all\n> would be able to spot constructs like this one, which are basically:\n>\n> while (1) {\n> \t...\n> if (x)\n> \treturn;\n> \t...\n> }\n> return; /* unreachable */\n>\n> Where the elided code contains no \"break\", \"goto\" or other mechanism for\n> exiting the for-loop.\n>\n> I.e. GCC and Clang don't bother to note the unreachable code, but I\n> don't think the reverse will be true, that a compiler will say that a\n> \"return\" is missing there. Having a function be just a loop body that\n> returns an some point is a common pattern.\n\nRight, but I'm more concerned about a less advanced compiler that would\ncomplain about the absence of a `return` statement as the last\ninstruction in a non-void function.\n\nThis is probably all academic, anyway, since less advanced compilers\nprobably have other issues compiling Git as it stands today, but\nfundamentally I think that SunCC's warnings here are at the very least\ninconsiderate of less advanced compilers.\n\nTo me, the safest thing to do would be to leave the code as-is and drop\nthis patch.\n\nThanks,\nTaylor\n"},{"id":"446115","messageId":"220113.867db4vtk6.gmgdl@evledraar.gmail.com","threadId":"57227","inReplyTo":"Yd8m2YeDZrgSiFzJ@nand.local","subject":"Re: [PATCH 3/3] reftable tests: avoid \"int\" overflow, use \"uint64_t\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-13T10:04:07Z","receivedAt":"2022-01-13T10:06:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 12 2022, Taylor Blau wrote:\n\n> On Wed, Jan 12, 2022 at 11:02:05AM -0800, Junio C Hamano wrote:\n>> Johannes Sixt writes:\n>>\n>> > Am 11.01.22 um 21:18 schrieb Taylor Blau:\n>> >> On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:\n>> >>> Am 11.01.22 um 20:41 schrieb Taylor Blau:\n>> >>>> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:\n>> >>>>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau wrote:\n>> >>>>>> In any case, you're only setting the lower half of `min` high. Maybe:\n>> >>>>>>\n>> >>>>>> uint64_t min = ~0ul;\n>> >>>>>\n>> >>>>> yeah, that works.\n>> >>>>\n>> >>>> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from\n>> >>>> somebody more confident than I in this area would be welcome :).\n>> >>>\n>> >>> It does not work on Windows: unsigned long is 32 bits wide. You have to\n>> >>> make it\n>> >>>\n>> >>> uint64_t min = ~(uint64_t)0;\n>> >>\n>> >> Perfect; this is exactly what I was looking for. Thanks!\n>>\n>> That sounds perfect.\n>>\n>> > Actually, on second thought, UINT64_MAX would be even better.\n>>\n>> I wouldn't introduce use of UINT64_MAX, which \"git grep\" does not\n>> produce any hits for.\n>\n>> Unless it is very early in a development cycle, that is, in which\n>> case we have enough time to help platforms that are not quite POSIX.\n>\n> Yep, I agree that avoiding introducing the first instance of UINT64_MAX\n> in our tree is worth doing (probably in general, but certainly now that\n> we're past even -rc0).\n>\n> Either `~(uint64_t)0` or `UINTMAX_MAX` would be fine with me.\n\nThe reason I left it at 0xffffffff is because the current test clearly\ndoesn't care about using the maximum width of the type, and I was just\ntrying to get rid of the associated compiler warning.\n\nSo I'll leave it to Han-Wen to comment on if the \"max\" being the maximum\nof the type is actually important here.\n\nAs far as what we'd pick to get the maximum type value goes, we should\njust prefer whatever we use for that already in that codebase, and we've\ngot this in a related file there:\n \n reftable/generic.c: struct reftable_log_record log = {\n reftable/generic.c- .refname = (char *)name,\n reftable/generic.c- .update_index = ~((uint64_t)0),\n reftable/generic.c- };\n\n(Which is what Johannes Sixt independently suggested upthread in\n<45baffd7-c9f3-cc52-47b4-ea0fee0182a8@kdbg.org>).\n"},{"id":"446116","messageId":"220113.8635lsvsw6.gmgdl@evledraar.gmail.com","threadId":"57227","inReplyTo":"Yd8njHz2m099iDfL@nand.local","subject":"Re: [PATCH 1/3] test-tool genzeros: initialize \"zeros\" to avoid SunCC warning","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-13T10:08:44Z","receivedAt":"2022-01-13T10:20:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 12 2022, Taylor Blau wrote:\n\n> On Wed, Jan 12, 2022 at 03:21:46PM +0100, Johannes Schindelin wrote:\n>> > diff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\n>> > index 8ca988d6216..5dc89eda0cb 100644\n>> > --- a/t/helper/test-genzeros.c\n>> > +++ b/t/helper/test-genzeros.c\n>> > @@ -3,8 +3,7 @@\n>> >\n>> > int cmd__genzeros(int argc, const char **argv)\n>> > {\n>> > -\t/* static, so that it is NUL-initialized */\n>> > -\tstatic const char zeros[256 * 1024];\n>> > +\tconst char zeros[256 * 1024] = { 0 };\n>>\n>> This diff does two things: add an initializer, and turn the variable into\n>> a `static`. The former is the actual fix that is required. The latter is\n>> not. During the -rc phase, we do not want to see any of the latter. It is\n>> unnecessarily controversial and distracting, and can easily be postponed\n>> until January 25th, 2022.\n>\n> This assumes that making the declaration non-static isn't necessary to\n> fix the warning from SunCC.\n\nJust adding \"= { 0 }\" and retaining the \"static\" would FWIW make SunCC\nhappy here.\n\n> I would guess that in reality it probably isn't, so removing the static\n> designation is a stray change, and this would have been easier to grok\n> as simply:\n>\n> -\tstatic const char zeros[256 * 1024];\n> +\tstatic const char zeros[256 * 1024] = { 0 };\n>\n> But to be honest I don't think it is _that_ big of a deal to make such a\n> small change during this point of the development cycle.\n\nWe'd also need to change the comment, so:\n\n-\t/* static, so that it is NUL-initialized */\n-\tstatic const char zeros[256 * 1024];\n+\t/* static, for no particular reason */\n+\tstatic const char zeros[256 * 1024] = { 0 };\n\nWhich is why I stripped the \"static\" off it, it was only there as a\nshorthand for doing the initialization, so once we're doing it ourselves\nit makes no sense to retain it for this invoked-only-once test helper.\n\nSo I think this patch is good as-is. just adding the initializer would\nneed even further explanation in the comment/commit message about the\nnon-sensical end-state. I'm all for being careful in the rc period, but\nin this case I think we'd be overdoing it.\n"},{"id":"446117","messageId":"220113.86y23jvrrw.gmgdl@evledraar.gmail.com","threadId":"57227","inReplyTo":"Yd8pzOh1HLvDWSPg@nand.local","subject":"Re: [PATCH 2/3] reftable: remove unreachable \"return\" statements","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-13T10:29:35Z","receivedAt":"2022-01-13T10:44:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 12 2022, Taylor Blau wrote:\n\n> On Wed, Jan 12, 2022 at 01:47:40PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> >> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> >> ---\n>> >> reftable/refname.c | 1 -\n>> >> reftable/writer.c | 1 -\n>> >> 2 files changed, 2 deletions(-)\n>> >>\n>> >> diff --git a/reftable/refname.c b/reftable/refname.c\n>> >> index 95734969324..136001bc2c7 100644\n>> >> --- a/reftable/refname.c\n>> >> +++ b/reftable/refname.c\n>> >> @@ -132,7 +132,6 @@ static int validate_refname(const char *name)\n>> >> \t\t\treturn REFTABLE_REFNAME_ERROR;\n>> >> \t\tname = next + 1;\n>> >> \t}\n>> >> -\treturn 0;\n>> >> }\n>> >\n>> > In this case the loop inside of validate_refname() should always\n>> > terminate the function within the loop body. But removing this return\n>> > statement here relies on the compiler to determine that fact.\n>> >\n>> > I could well imagine on the other end of the spectrum there exists a\n>> > compiler which _doesn't_ make this inference pass, and would complain\n>> > about the opposite thing as you're reporting from SunCC (i.e., that this\n>> > function which returns something other than void does not have a return\n>> > statement outside of the loop).\n>> >\n>> > So in that sense, I disagree with the guidance of SunCC's warning. In\n>> > other words: by quelching this warning under one compiler, are we\n>> > introducing a new warning under a different/less advanced compiler?\n>>\n>> I'd think that any compiler who'd warn about this sort of thing at all\n>> would be able to spot constructs like this one, which are basically:\n>>\n>> while (1) {\n>> \t...\n>> if (x)\n>> \treturn;\n>> \t...\n>> }\n>> return; /* unreachable */\n>>\n>> Where the elided code contains no \"break\", \"goto\" or other mechanism for\n>> exiting the for-loop.\n>>\n>> I.e. GCC and Clang don't bother to note the unreachable code, but I\n>> don't think the reverse will be true, that a compiler will say that a\n>> \"return\" is missing there. Having a function be just a loop body that\n>> returns an some point is a common pattern.\n>\n> Right, but I'm more concerned about a less advanced compiler that would\n> complain about the absence of a `return` statement as the last\n> instruction in a non-void function.\n>\n> This is probably all academic, anyway, since less advanced compilers\n> probably have other issues compiling Git as it stands today, but\n> fundamentally I think that SunCC's warnings here are at the very least\n> inconsiderate of less advanced compilers.\n>\n> To me, the safest thing to do would be to leave the code as-is and drop\n> this patch.\n\nI really don't see that, sorry. We have an actual example of a compliler\nthat does emit a warning new in this rc on this code, but AFAICT your\nconcern is purely hypothetical.\n\nSuch a hypothetical compiler would already be emitting a firehose of\nfalse-positive warnings in our or any non-trivial C codebase,\ne.g. builtin/bisect--helper.c:bisect_run(), show-branch.c:version_cmp()\nand fsck.c:count_leading_dotdots() would all warn (and I just picked\nthree examples from a quick grep, there's a lot more of them).\n\nSo I don't think we need to be concerned about such a hypothetical\ncompiler. I think anyone doing such flow analysis tries to do it well\nenough to not make the warning entirely useless.\n\nAside: SunCC does get it wrong in some cases, but it's more obscure\ncode, mainly from jumping into a for-loop via \"goto\", and not\npropagating understanding the implications of NORETURN in some cases (or\nmaybe we're just using the GCC-specific one in that case).\n"},{"id":"446156","messageId":"nycvar.QRO.7.76.6.2201131629300.2121@tvgsbejvaqbjf.bet","threadId":"57227","inReplyTo":"220113.8635lsvsw6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/3] test-tool genzeros: initialize \"zeros\" to avoid SunCC warning","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-13T15:31:37Z","receivedAt":"2022-01-13T15:31:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Thu, 13 Jan 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> On Wed, Jan 12 2022, Taylor Blau wrote:\n>\n> > On Wed, Jan 12, 2022 at 03:21:46PM +0100, Johannes Schindelin wrote:\n> >> > diff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\n> >> > index 8ca988d6216..5dc89eda0cb 100644\n> >> > --- a/t/helper/test-genzeros.c\n> >> > +++ b/t/helper/test-genzeros.c\n> >> > @@ -3,8 +3,7 @@\n> >> >\n> >> > int cmd__genzeros(int argc, const char **argv)\n> >> > {\n> >> > -\t/* static, so that it is NUL-initialized */\n> >> > -\tstatic const char zeros[256 * 1024];\n> >> > +\tconst char zeros[256 * 1024] = { 0 };\n> >>\n> >> This diff does two things: add an initializer, and turn the variable into\n> >> a `static`. The former is the actual fix that is required. The latter is\n> >> not. During the -rc phase, we do not want to see any of the latter. It is\n> >> unnecessarily controversial and distracting, and can easily be postponed\n> >> until January 25th, 2022.\n> >\n> > This assumes that making the declaration non-static isn't necessary to\n> > fix the warning from SunCC.\n>\n> Just adding \"= { 0 }\" and retaining the \"static\" would FWIW make SunCC\n> happy here.\n\nProbably moving the `static` outside of the function would do the same,\nand would be even more surgical a change.\n\n> > I would guess that in reality it probably isn't, so removing the static\n> > designation is a stray change, and this would have been easier to grok\n> > as simply:\n> >\n> > -\tstatic const char zeros[256 * 1024];\n> > +\tstatic const char zeros[256 * 1024] = { 0 };\n> >\n> > But to be honest I don't think it is _that_ big of a deal to make such a\n> > small change during this point of the development cycle.\n>\n> We'd also need to change the comment, so:\n>\n> -\t/* static, so that it is NUL-initialized */\n> -\tstatic const char zeros[256 * 1024];\n> +\t/* static, for no particular reason */\n> +\tstatic const char zeros[256 * 1024] = { 0 };\n>\n> Which is why I stripped the \"static\" off it, it was only there as a\n> shorthand for doing the initialization, so once we're doing it ourselves\n> it makes no sense to retain it for this invoked-only-once test helper.\n>\n> So I think this patch is good as-is.\n\nThat's stating the obvious.\n\n> just adding the initializer would need even further explanation in the\n> comment/commit message about the non-sensical end-state. I'm all for\n> being careful in the rc period, but in this case I think we'd be\n> overdoing it.\n\nIf this were the only instance where the patch is larger than strictly\nnecessary in the -rc phase, I would have let it slide, too.\n\nCiao,\nJohannes\n"},{"id":"446157","messageId":"nycvar.QRO.7.76.6.2201131638500.2121@tvgsbejvaqbjf.bet","threadId":"57227","inReplyTo":"220113.86y23jvrrw.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/3] reftable: remove unreachable \"return\" statements","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-13T15:39:31Z","receivedAt":"2022-01-13T15:39:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Thu, 13 Jan 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> On Wed, Jan 12 2022, Taylor Blau wrote:\n>\n> > On Wed, Jan 12, 2022 at 01:47:40PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> >> >> Signed-off-by: Ævar Arnfjörð Bjarmason \n> >> >> ---\n> >> >> reftable/refname.c | 1 -\n> >> >> reftable/writer.c | 1 -\n> >> >> 2 files changed, 2 deletions(-)\n> >> >>\n> >> >> diff --git a/reftable/refname.c b/reftable/refname.c\n> >> >> index 95734969324..136001bc2c7 100644\n> >> >> --- a/reftable/refname.c\n> >> >> +++ b/reftable/refname.c\n> >> >> @@ -132,7 +132,6 @@ static int validate_refname(const char *name)\n> >> >> \t\t\treturn REFTABLE_REFNAME_ERROR;\n> >> >> \t\tname = next + 1;\n> >> >> \t}\n> >> >> -\treturn 0;\n> >> >> }\n> >> >\n> >> > In this case the loop inside of validate_refname() should always\n> >> > terminate the function within the loop body. But removing this return\n> >> > statement here relies on the compiler to determine that fact.\n> >> >\n> >> > I could well imagine on the other end of the spectrum there exists a\n> >> > compiler which _doesn't_ make this inference pass, and would complain\n> >> > about the opposite thing as you're reporting from SunCC (i.e., that this\n> >> > function which returns something other than void does not have a return\n> >> > statement outside of the loop).\n> >> >\n> >> > So in that sense, I disagree with the guidance of SunCC's warning. In\n> >> > other words: by quelching this warning under one compiler, are we\n> >> > introducing a new warning under a different/less advanced compiler?\n> >>\n> >> I'd think that any compiler who'd warn about this sort of thing at all\n> >> would be able to spot constructs like this one, which are basically:\n> >>\n> >> while (1) {\n> >> \t...\n> >> if (x)\n> >> \treturn;\n> >> \t...\n> >> }\n> >> return; /* unreachable */\n> >>\n> >> Where the elided code contains no \"break\", \"goto\" or other mechanism for\n> >> exiting the for-loop.\n> >>\n> >> I.e. GCC and Clang don't bother to note the unreachable code, but I\n> >> don't think the reverse will be true, that a compiler will say that a\n> >> \"return\" is missing there. Having a function be just a loop body that\n> >> returns an some point is a common pattern.\n> >\n> > Right, but I'm more concerned about a less advanced compiler that would\n> > complain about the absence of a `return` statement as the last\n> > instruction in a non-void function.\n> >\n> > This is probably all academic, anyway, since less advanced compilers\n> > probably have other issues compiling Git as it stands today, but\n> > fundamentally I think that SunCC's warnings here are at the very least\n> > inconsiderate of less advanced compilers.\n> >\n> > To me, the safest thing to do would be to leave the code as-is and drop\n> > this patch.\n>\n> I really don't see that, sorry. We have an actual example of a compliler\n> that does emit a warning new in this rc on this code, but AFAICT your\n> concern is purely hypothetical.\n\nIt's just a warning.\n\nSo I concur with Taylor. This patch does not need to go into v2.35.0.\n\nCiao,\nJohannes\n"},{"id":"446168","messageId":"xmqqee5bk02j.fsf@gitster.g","threadId":"57227","inReplyTo":"220113.8635lsvsw6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/3] test-tool genzeros: initialize \"zeros\" to avoid SunCC warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-13T17:38:44Z","receivedAt":"2022-01-13T17:38:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Wed, Jan 12 2022, Taylor Blau wrote:\n>\n>> On Wed, Jan 12, 2022 at 03:21:46PM +0100, Johannes Schindelin wrote:\n>>> > diff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c\n>>> > index 8ca988d6216..5dc89eda0cb 100644\n>>> > --- a/t/helper/test-genzeros.c\n>>> > +++ b/t/helper/test-genzeros.c\n>>> > @@ -3,8 +3,7 @@\n>>> >\n>>> > int cmd__genzeros(int argc, const char **argv)\n>>> > {\n>>> > -\t/* static, so that it is NUL-initialized */\n>>> > -\tstatic const char zeros[256 * 1024];\n>>> > +\tconst char zeros[256 * 1024] = { 0 };\n>>>\n>>> This diff does two things: add an initializer, and turn the variable into\n>>> a `static`. The former is the actual fix that is required. The latter is\n>>> not. During the -rc phase, we do not want to see any of the latter. It is\n>>> unnecessarily controversial and distracting, and can easily be postponed\n>>> until January 25th, 2022.\n>>\n>> This assumes that making the declaration non-static isn't necessary to\n>> fix the warning from SunCC.\n>\n> Just adding \"= { 0 }\" and retaining the \"static\" would FWIW make SunCC\n> happy here.\n\nIt would make folks, who worry about having too large an item on the\nstack to begin with, happy, too. 256kB on stack of a function that\ndoes not make a call into a deep call chain would not matter all\nthat much, but it is a good principle to keep in mind.\n\nWe've worked around false \"uninitialized\" alarms from too picky\n(versions of) compilers before by adding an otherwise unnecessary\ninitializers before, and I think this falls into the same category.\n\nIt is a separate matter if it is appropriate to worry about SunCC\nthis late in the cycle. If this were \"we were clean before, and\nthese small number of places add breakages\", I would say yes.\n\nBut if this is \"let's not add more of the same existing breakage\nthat we already have tons\", we should not even be discussing about\nsuch a change this late in the cycle (immediately after the new\noffenders were added would have been more appropriate).\n\nI offhand do not know which side of that line this one falls,\nthough.\n\nThanks.\n\n"},{"id":"446180","messageId":"9ce034fd-b696-60d2-c292-98285aff180a@kdbg.org","threadId":"57227","inReplyTo":"220112.865yqpxge2.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/3] reftable: remove unreachable \"return\" statements","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2022-01-13T20:17:30Z","receivedAt":"2022-01-13T20:17:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12.01.22 um 13:47 schrieb Ævar Arnfjörð Bjarmason:\n> \n> On Tue, Jan 11 2022, Taylor Blau wrote:\n> \n>> On Tue, Jan 11, 2022 at 05:40:22PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>>> Remove unreachable return statements added in acb533440fc (reftable:\n>>> implement refname validation, 2021-10-07) and f14bd719349 (reftable:\n>>> write reftable files, 2021-10-07).\n>>>\n>>> This avoids the following warnings on SunCC 12.5 on\n>>> gcc211.fsffrance.org:\n>>>\n>>> \"reftable/refname.c\", line 135: warning: statement not reached\n>>> \"reftable/refname.c\", line 135: warning: statement not reached\n>>\n>> Interesting. From a cursory reading, I agree with the assessment of\n>> at least my compiler that these return statements are both unnecessary,\n>> but...\n>>\n>>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>>> ---\n>>> reftable/refname.c | 1 -\n>>> reftable/writer.c | 1 -\n>>> 2 files changed, 2 deletions(-)\n>>>\n>>> diff --git a/reftable/refname.c b/reftable/refname.c\n>>> index 95734969324..136001bc2c7 100644\n>>> --- a/reftable/refname.c\n>>> +++ b/reftable/refname.c\n>>> @@ -132,7 +132,6 @@ static int validate_refname(const char *name)\n>>> \t\t\treturn REFTABLE_REFNAME_ERROR;\n>>> \t\tname = next + 1;\n>>> \t}\n>>> -\treturn 0;\n>>> }\n>>\n>> In this case the loop inside of validate_refname() should always\n>> terminate the function within the loop body. But removing this return\n>> statement here relies on the compiler to determine that fact.\n>>\n>> I could well imagine on the other end of the spectrum there exists a\n>> compiler which _doesn't_ make this inference pass, and would complain\n>> about the opposite thing as you're reporting from SunCC (i.e., that this\n>> function which returns something other than void does not have a return\n>> statement outside of the loop).\n>>\n>> So in that sense, I disagree with the guidance of SunCC's warning. In\n>> other words: by quelching this warning under one compiler, are we\n>> introducing a new warning under a different/less advanced compiler?\n> \n> I'd think that any compiler who'd warn about this sort of thing at all\n> would be able to spot constructs like this one, which are basically:\n> \n> while (1) {\n> \t...\n> if (x)\n> \treturn;\n> \t...\n> }\n> return; /* unreachable */\n> \n> Where the elided code contains no \"break\", \"goto\" or other mechanism for\n> exiting the for-loop.\n\nWhy not just sidestep the problematic case:\n\ndiff --git a/reftable/refname.c b/reftable/refname.c\nindex 9573496932..4f89956187 100644\n--- a/reftable/refname.c\n+++ b/reftable/refname.c\n@@ -120,17 +120,17 @@ static int modification_has_ref_with_prefix(struct modification *mod,\n static int validate_refname(const char *name)\n {\n \twhile (1) {\n \t\tchar *next = strchr(name, '/');\n \t\tif (!*name) {\n \t\t\treturn REFTABLE_REFNAME_ERROR;\n \t\t}\n \t\tif (!next) {\n-\t\t\treturn 0;\n+\t\t\tbreak;\n \t\t}\n \t\tif (next - name == 0 || (next - name == 1 && *name == '.') ||\n \t\t (next - name == 2 && name[0] == '.' && name[1] == '.'))\n \t\t\treturn REFTABLE_REFNAME_ERROR;\n \t\tname = next + 1;\n \t}\n \treturn 0;\n }\n\nSure, there are returns in the loop, but they are clearly error cases.\nThe regular exit is now at the end of the function.\n\n-- Hannes\n"},{"id":"446184","messageId":"xmqqlezjgvwb.fsf@gitster.g","threadId":"57227","inReplyTo":"9ce034fd-b696-60d2-c292-98285aff180a@kdbg.org","subject":"Re: [PATCH 2/3] reftable: remove unreachable \"return\" statements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-13T21:37:08Z","receivedAt":"2022-01-13T21:37:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Why not just sidestep the problematic case:\n>\n> diff --git a/reftable/refname.c b/reftable/refname.c\n> index 9573496932..4f89956187 100644\n> --- a/reftable/refname.c\n> +++ b/reftable/refname.c\n> @@ -120,17 +120,17 @@ static int modification_has_ref_with_prefix(struct modification *mod,\n> static int validate_refname(const char *name)\n> {\n> \twhile (1) {\n> \t\tchar *next = strchr(name, '/');\n> \t\tif (!*name) {\n> \t\t\treturn REFTABLE_REFNAME_ERROR;\n> \t\t}\n> \t\tif (!next) {\n> -\t\t\treturn 0;\n> +\t\t\tbreak;\n> \t\t}\n> \t\tif (next - name == 0 || (next - name == 1 && *name == '.') ||\n> \t\t (next - name == 2 && name[0] == '.' && name[1] == '.'))\n> \t\t\treturn REFTABLE_REFNAME_ERROR;\n> \t\tname = next + 1;\n> \t}\n> \treturn 0;\n> }\n>\n> Sure, there are returns in the loop, but they are clearly error cases.\n> The regular exit is now at the end of the function.\n\n;-)\n"},{"id":"446185","messageId":"xmqqh7a7gvtc.fsf@gitster.g","threadId":"57227","inReplyTo":"220113.867db4vtk6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/3] reftable tests: avoid \"int\" overflow, use \"uint64_t\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-13T21:38:55Z","receivedAt":"2022-01-13T21:38:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> The reason I left it at 0xffffffff is because the current test clearly\n> doesn't care about using the maximum width of the type, and I was just\n> trying to get rid of the associated compiler warning.\n\nSounds sane. Let's take the original one you sent out, and those\nwho want to make things consistent can swap it with ~((uint64_t)0)\nafter the release.\n\nThanks.\n"}]}