{"thread":{"id":"51083","subject":"[PATCH 0/2] pkt-line: fix incorrect function declaration","startedAt":"2019-05-13T22:43:19Z","lastAt":"2019-05-17T18:54:03Z","messageCount":15,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"375427","messageId":"pull.192.git.gitgitgadget@gmail.com","threadId":"51083","inReplyTo":null,"subject":"[PATCH 0/2] pkt-line: fix incorrect function declaration","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-13T22:43:15Z","receivedAt":"2019-05-13T22:43:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"MS Visual C detected a mismatch between the declaration and the definition\nof set_packet_header(): it is declared with its second parameter missing the \nconst attribute.\n\nIt also detected a mismatch between the declaration and the definition of \nparse_opt_unknown_cb().\n\nThese problems must have been introduced very recently; I do not recall\nseeing them before today in any of Git for Windows' ever-green branches\n(i.e. master semi-automatically rebased continously onto pu, next, master \nand maint).\n\nYou could not have seen it in git.git's own Azure Pipeline, as Git for\nWindows' version already has support to build with MSVC (I plan to submit\nthis directly after v2.22.0 is out).\n\nJohannes Schindelin (2):\n pkt-line: fix declaration of `set_packet_header()`\n parse-options: adjust `parse_opt_unknown_cb()`s declared return type\n\n parse-options.h | 4 +++-\n pkt-line.h | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\n\nbase-commit: ab15ad1a3b4b04a29415aef8c9afa2f64fc194a2\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-192%2Fdscho%2Ffix-set_packet_header-signature-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-192/dscho/fix-set_packet_header-signature-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/192\n-- \ngitgitgadget\n"},{"id":"375428","messageId":"a6bfec76c85bbe9187b536ff78252b82e30e20d3.1557787395.git.gitgitgadget@gmail.com","threadId":"51083","inReplyTo":"pull.192.git.gitgitgadget@gmail.com","subject":"[PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-13T22:43:16Z","receivedAt":"2019-05-13T22:43:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen this function was changed in a97d00799a19 (remote-curl: use\npost_rpc() for protocol v2 also, 2019-02-21) from file-local to global,\nthe declaration was incorrectly missing the `const` qualifier.\n\nLet's fix that.\n\nSigned-off-by: Johannes Schindelin \n---\n pkt-line.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 5c62015db4..04ae7d802c 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -25,7 +25,7 @@ void packet_delim(int fd);\n void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n void packet_buf_flush(struct strbuf *buf);\n void packet_buf_delim(struct strbuf *buf);\n-void set_packet_header(char *buf, int size);\n+void set_packet_header(char *buf, const int size);\n void packet_write(int fd_out, const char *buf, size_t size);\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len);\n-- \ngitgitgadget\n\n"},{"id":"375429","messageId":"dd283736b72d3e52bd298b0360ab66ffd15c1cf9.1557787395.git.gitgitgadget@gmail.com","threadId":"51083","inReplyTo":"pull.192.git.gitgitgadget@gmail.com","subject":"[PATCH 2/2] parse-options: adjust `parse_opt_unknown_cb()`s declared return type","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-13T22:43:17Z","receivedAt":"2019-05-13T22:43:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn f41179f16ba2 (parse-options: avoid magic return codes, 2019-01-27),\nthe signature of the low-level parse-opt callback function was changed\nto return an `enum`.\n\nAnd while the implementations were changed, one declaration was left\nunchanged, still claiming to return `int`.\n\nThis can potentially lead to problems, as compilers are free to choose\nany integral type for an `enum` as long as it can represent all declared\nvalues.\n\nSigned-off-by: Johannes Schindelin \n---\n parse-options.h | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options.h b/parse-options.h\nindex bd00cf0049..cd756833a9 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -286,7 +286,9 @@ int parse_opt_commit(const struct option *, const char *, int);\n int parse_opt_tertiary(const struct option *, const char *, int);\n int parse_opt_string_list(const struct option *, const char *, int);\n int parse_opt_noop_cb(const struct option *, const char *, int);\n-int parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx, const struct option *, const char *, int);\n+enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,\n+\t\t\t\t\t const struct option *,\n+\t\t\t\t\t const char *, int);\n int parse_opt_passthru(const struct option *, const char *, int);\n int parse_opt_passthru_argv(const struct option *, const char *, int);\n \n-- \ngitgitgadget\n"},{"id":"375439","messageId":"xmqqo9469buw.fsf@gitster-ct.c.googlers.com","threadId":"51083","inReplyTo":"a6bfec76c85bbe9187b536ff78252b82e30e20d3.1557787395.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-13T23:24:39Z","receivedAt":"2019-05-13T23:24:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> When this function was changed in a97d00799a19 (remote-curl: use\n> post_rpc() for protocol v2 also, 2019-02-21) from file-local to global,\n> the declaration was incorrectly missing the `const` qualifier.\n\nI do not quite get it. Back when the function was file-scope\nstatic, it did not even have a separate declaration, and the\ndefinition the said commit added looks correct to me.\n\nHaving \"const int size\" parameter in the definition of a function\ndoes help the compilers and the developers by making sure any\nearlier reference to the parameter in the function would not modify\nit and cause later reference to obtain a different value.\n\nBut the parameter treated as a constant without getting modified\nduring the invocation of the function is an implementation detail of\nthe function; there is no point exposing that implementation detail\nto its callers. It does not even help the compilers handling the\ncaller's compilation unit---the parameter is passed by value, so the\ncaller knows that the callee would not modify it without \"const\"\nthere.\n\nDoes the language even allow flagging \"const int in the definition,\nint in the declaration\" as a warning-worthy discrepancy?\n\n> -void set_packet_header(char *buf, int size);\n> +void set_packet_header(char *buf, const int size);\n"},{"id":"375440","messageId":"xmqqk1eu9bnf.fsf@gitster-ct.c.googlers.com","threadId":"51083","inReplyTo":"dd283736b72d3e52bd298b0360ab66ffd15c1cf9.1557787395.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] parse-options: adjust `parse_opt_unknown_cb()`s declared return type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-13T23:29:08Z","receivedAt":"2019-05-13T23:29:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In f41179f16ba2 (parse-options: avoid magic return codes, 2019-01-27),\n> the signature of the low-level parse-opt callback function was changed\n> to return an `enum`.\n>\n> And while the implementations were changed, one declaration was left\n> unchanged, still claiming to return `int`.\n>\n> This can potentially lead to problems, as compilers are free to choose\n> any integral type for an `enum` as long as it can represent all declared\n> values.\n\nThe enum is meant to represent \"these magic negative numbers\", and\nif the compiler chose \"long\" for it and the implementation of the\nfunction returned a \"long\", while the caller thought it would yield\nan \"int\", things will break.\n\nLooks good to make callers' expectation and what callee does\nconsistent.\n\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> parse-options.h | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/parse-options.h b/parse-options.h\n> index bd00cf0049..cd756833a9 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -286,7 +286,9 @@ int parse_opt_commit(const struct option *, const char *, int);\n> int parse_opt_tertiary(const struct option *, const char *, int);\n> int parse_opt_string_list(const struct option *, const char *, int);\n> int parse_opt_noop_cb(const struct option *, const char *, int);\n> -int parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx, const struct option *, const char *, int);\n> +enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,\n> +\t\t\t\t\t const struct option *,\n> +\t\t\t\t\t const char *, int);\n> int parse_opt_passthru(const struct option *, const char *, int);\n> int parse_opt_passthru_argv(const struct option *, const char *, int);\n"},{"id":"375530","messageId":"nycvar.QRO.7.76.6.1905141454510.44@tvgsbejvaqbjf.bet","threadId":"51083","inReplyTo":"xmqqo9469buw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-14T12:57:01Z","receivedAt":"2019-05-14T12:57:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 14 May 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > When this function was changed in a97d00799a19 (remote-curl: use\n> > post_rpc() for protocol v2 also, 2019-02-21) from file-local to global,\n> > the declaration was incorrectly missing the `const` qualifier.\n>\n> I do not quite get it. Back when the function was file-scope\n> static, it did not even have a separate declaration, and the\n> definition the said commit added looks correct to me.\n\nRight, I should have said \"the new declaration\" instead of \"the\ndeclaration\".\n\n> Having \"const int size\" parameter in the definition of a function\n> does help the compilers and the developers by making sure any\n> earlier reference to the parameter in the function would not modify\n> it and cause later reference to obtain a different value.\n>\n> But the parameter treated as a constant without getting modified\n> during the invocation of the function is an implementation detail of\n> the function; there is no point exposing that implementation detail\n> to its callers. It does not even help the compilers handling the\n> caller's compilation unit---the parameter is passed by value, so the\n> caller knows that the callee would not modify it without \"const\"\n> there.\n>\n> Does the language even allow flagging \"const int in the definition,\n> int in the declaration\" as a warning-worthy discrepancy?\n\nApparently it does, as MS Visual C does issue a warning (and with `/Wall`,\nit fails).\n\nIn any case, I don't think that it makes sense to have a function\ndeclaration whose signature differs from the definition's.\n\nCiao,\nDscho\n\n> > -void set_packet_header(char *buf, int size);\n> > +void set_packet_header(char *buf, const int size);\n>\n"},{"id":"375539","messageId":"20190514144305.GA28530@sigill.intra.peff.net","threadId":"51083","inReplyTo":"nycvar.QRO.7.76.6.1905141454510.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T14:43:06Z","receivedAt":"2019-05-14T14:43:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 14, 2019 at 02:57:01PM +0200, Johannes Schindelin wrote:\n\n> > But the parameter treated as a constant without getting modified\n> > during the invocation of the function is an implementation detail of\n> > the function; there is no point exposing that implementation detail\n> > to its callers. It does not even help the compilers handling the\n> > caller's compilation unit---the parameter is passed by value, so the\n> > caller knows that the callee would not modify it without \"const\"\n> > there.\n> >\n> > Does the language even allow flagging \"const int in the definition,\n> > int in the declaration\" as a warning-worthy discrepancy?\n> \n> Apparently it does, as MS Visual C does issue a warning (and with `/Wall`,\n> it fails).\n> \n> In any case, I don't think that it makes sense to have a function\n> declaration whose signature differs from the definition's.\n\nI actually agree with Junio's point that in an ideal world the\ndeclaration should omit details that are not relevant to the caller. But\nclearly we do not live in that world, and this is a small enough point\nthat we should fix it in one direction or the other.\n\nI do have a slight preference for going the _other_ way. There is no\nneed to mark the parameter as const in the definition. It is passed by\nvalue, so nobody except the function body cares either way. And we have\nmany function bodies where value-passed parameters (or local variables!)\nare not marked as const, even though they are only assigned to once.\n\nI don't think that annotation is telling much to any reader of the code,\nnor to a decent optimizing compiler.\n\n-Peff\n"},{"id":"375540","messageId":"20190514144439.GB28530@sigill.intra.peff.net","threadId":"51083","inReplyTo":"20190514144305.GA28530@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T14:44:39Z","receivedAt":"2019-05-14T14:44:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 14, 2019 at 10:43:06AM -0400, Jeff King wrote:\n\n> On Tue, May 14, 2019 at 02:57:01PM +0200, Johannes Schindelin wrote:\n> \n> > > But the parameter treated as a constant without getting modified\n> > > during the invocation of the function is an implementation detail of\n> > > the function; there is no point exposing that implementation detail\n> > > to its callers. It does not even help the compilers handling the\n> > > caller's compilation unit---the parameter is passed by value, so the\n> > > caller knows that the callee would not modify it without \"const\"\n> > > there.\n> > >\n> > > Does the language even allow flagging \"const int in the definition,\n> > > int in the declaration\" as a warning-worthy discrepancy?\n> > \n> > Apparently it does, as MS Visual C does issue a warning (and with `/Wall`,\n> > it fails).\n> > \n> > In any case, I don't think that it makes sense to have a function\n> > declaration whose signature differs from the definition's.\n> \n> I actually agree with Junio's point that in an ideal world the\n> declaration should omit details that are not relevant to the caller. But\n> clearly we do not live in that world, and this is a small enough point\n> that we should fix it in one direction or the other.\n> \n> I do have a slight preference for going the _other_ way. There is no\n> need to mark the parameter as const in the definition. It is passed by\n> value, so nobody except the function body cares either way. And we have\n> many function bodies where value-passed parameters (or local variables!)\n> are not marked as const, even though they are only assigned to once.\n> \n> I don't think that annotation is telling much to any reader of the code,\n> nor to a decent optimizing compiler.\n\nTo be clear, I can live with your patch as-is, and I don't think this\none site overly matters. Mostly I do not want to see \"let's declare\npass-by-value parameters as const\" picked up as a general concept, and\nit seems easiest to try to squash the first instance of it. :)\n\n-Peff\n"},{"id":"375584","messageId":"xmqqa7fo8pdg.fsf@gitster-ct.c.googlers.com","threadId":"51083","inReplyTo":"20190514144305.GA28530@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-15T01:42:35Z","receivedAt":"2019-05-15T01:42:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I do have a slight preference for going the _other_ way. There is no\n> need to mark the parameter as const in the definition. It is passed by\n> value, so nobody except the function body cares either way. And we have\n> many function bodies where value-passed parameters (or local variables!)\n> are not marked as const, even though they are only assigned to once.\n\nThat would be more like this patch, then?\n\n-- >8 --\nSubject: pkt-line: drop 'const'-ness of a param to set_packet_header() \n\nThe fact that the incoming parameter is used as read-only in the\nfuction is an implementation detail that the callers should not have\nto know, and the prototype defined for the function in pkt-line.h\nlacked the \"const\" for that reason, but apparently some compilers\ncomplain about the parameter type mismatch.\n\nLet's squelch it by removing the \"const\" that is pointless for a\nsmall function like this, which would not help optimizing compilers\nnor reading humans that much.\n\nNoticed-by: Johannes Schindelin \nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n pkt-line.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex c9ed780d0b..a0e87b1e81 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -119,7 +119,7 @@ void packet_buf_delim(struct strbuf *buf)\n \tstrbuf_add(buf, \"0001\", 4);\n }\n \n-void set_packet_header(char *buf, const int size)\n+void set_packet_header(char *buf, int size)\n {\n \tstatic char hexchar[] = \"0123456789abcdef\";\n \n"},{"id":"375585","messageId":"20190515014440.GA13255@sigill.intra.peff.net","threadId":"51083","inReplyTo":"xmqqa7fo8pdg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-15T01:44:41Z","receivedAt":"2019-05-15T01:44:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 15, 2019 at 10:42:35AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I do have a slight preference for going the _other_ way. There is no\n> > need to mark the parameter as const in the definition. It is passed by\n> > value, so nobody except the function body cares either way. And we have\n> > many function bodies where value-passed parameters (or local variables!)\n> > are not marked as const, even though they are only assigned to once.\n> \n> That would be more like this patch, then?\n> \n> -- >8 --\n> Subject: pkt-line: drop 'const'-ness of a param to set_packet_header() \n> [..]\n\nYes, exactly.\n\n-Peff\n"},{"id":"375618","messageId":"nycvar.QRO.7.76.6.1905151234370.44@tvgsbejvaqbjf.bet","threadId":"51083","inReplyTo":"xmqqa7fo8pdg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-15T10:39:04Z","receivedAt":"2019-05-15T10:39:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 15 May 2019, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n> > I do have a slight preference for going the _other_ way. There is no\n> > need to mark the parameter as const in the definition. It is passed by\n> > value, so nobody except the function body cares either way. And we have\n> > many function bodies where value-passed parameters (or local variables!)\n> > are not marked as const, even though they are only assigned to once.\n>\n> That would be more like this patch, then?\n\nI can live very well with this patch.\n\nJust two minor comments:\n\n> -- >8 --\n> Subject: pkt-line: drop 'const'-ness of a param to set_packet_header()\n>\n> The fact that the incoming parameter is used as read-only in the\n> fuction is an implementation detail that the callers should not have\n> to know, and the prototype defined for the function in pkt-line.h\n> lacked the \"const\" for that reason, but apparently some compilers\n> complain about the parameter type mismatch.\n\nWe could be more explicit, as we know exactly that it is MS Visual C 2017\nthat is complaining.\n\n> Let's squelch it by removing the \"const\" that is pointless for a\n> small function like this, which would not help optimizing compilers\n\nIt is not pointless because of the size of the function, but because `int`\nis already a type that is always passed by value, never by reference.\n\nIf at all, it would make a difference if the function was inlined, so I\nwould argue that it would be pointless for large functions.\n\nBut both aspects are moot and more philosophical than anything else in\nthis context.\n\nThanks,\nDscho\n\n> nor reading humans that much.\n>\n> Noticed-by: Johannes Schindelin \n> Helped-by: Jeff King \n> Signed-off-by: Junio C Hamano \n> ---\n> pkt-line.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/pkt-line.c b/pkt-line.c\n> index c9ed780d0b..a0e87b1e81 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -119,7 +119,7 @@ void packet_buf_delim(struct strbuf *buf)\n> \tstrbuf_add(buf, \"0001\", 4);\n> }\n>\n> -void set_packet_header(char *buf, const int size)\n> +void set_packet_header(char *buf, int size)\n> {\n> \tstatic char hexchar[] = \"0123456789abcdef\";\n>\n>\n"},{"id":"375687","messageId":"xmqq36lf5e7a.fsf@gitster-ct.c.googlers.com","threadId":"51083","inReplyTo":"nycvar.QRO.7.76.6.1905151234370.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-16T02:24:25Z","receivedAt":"2019-05-16T02:24:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> lacked the \"const\" for that reason, but apparently some compilers\n>> complain about the parameter type mismatch.\n>\n> We could be more explicit, as we know exactly that it is MS Visual C 2017\n> that is complaining.\n\nWe could be, but I do not see a point of shaming one particular\ncompiler vendor.\n\n>> Let's squelch it by removing the \"const\" that is pointless for a\n>> small function like this, which would not help optimizing compilers\n>\n> It is not pointless because of the size of the function, but because `int`\n> is already a type that is always passed by value, never by reference.\n\nYou are looking only at the prototype side (i.e. declaration in\n*.h). Yes it is pointless for the callers. For the callee, the\nstory is different and pass-by-value does not even get in the\npicture.\n\nThe mention of pointless-ness I made was about the implementation\nside (i.e. definition in *.c). For a sufficiently large and complex\nfunction implementation, being able to say upfront that this\nincoming parameter is never modified would help following the logic\nin the implementation, so \"const int param\" in the parameter list of\na definition is *not* pointless in general. But apparently some\ncompilers are not happy abot \"const int param\" in implementation\nthat is paired with \"int param\" in prototype, we are dropping a\nconst that could be useful, with an excuse that for this particular\nfunction that is small and trivial, we can live without.\n\n"},{"id":"375694","messageId":"20190516034220.GE4596@sigill.intra.peff.net","threadId":"51083","inReplyTo":"xmqq36lf5e7a.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-16T03:42:20Z","receivedAt":"2019-05-16T03:42:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 16, 2019 at 11:24:25AM +0900, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> lacked the \"const\" for that reason, but apparently some compilers\n> >> complain about the parameter type mismatch.\n> >\n> > We could be more explicit, as we know exactly that it is MS Visual C 2017\n> > that is complaining.\n> \n> We could be, but I do not see a point of shaming one particular\n> compiler vendor.\n\nI'd slightly disagree. If 10 years from now we decide that MSVC is not a\nsupported compiler anymore, that information would be useful for\nsomebody digging into the history to say \"ah, we can probably use this\npattern once more\". I say probably because while it was the only\ncomplainer, once the pattern is removed we wouldn't know if anybody in\nthe interim _would_ have complained. But it still seems like something\nthat _could_ be useful to an archaeologist in the future.\n\n-Peff\n"},{"id":"375698","messageId":"xmqqtvdv3twf.fsf@gitster-ct.c.googlers.com","threadId":"51083","inReplyTo":"20190516034220.GE4596@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-16T04:28:16Z","receivedAt":"2019-05-16T04:31:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, May 16, 2019 at 11:24:25AM +0900, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>> \n>> >> lacked the \"const\" for that reason, but apparently some compilers\n>> >> complain about the parameter type mismatch.\n>> >\n>> > We could be more explicit, as we know exactly that it is MS Visual C 2017\n>> > that is complaining.\n>> \n>> We could be, but I do not see a point of shaming one particular\n>> compiler vendor.\n>\n> I'd slightly disagree. If 10 years from now we decide that MSVC is not a\n> supported compiler anymore, that information would be useful for\n> somebody digging into the history to say \"ah, we can probably use this\n> pattern once more\". I say probably because while it was the only\n> complainer, once the pattern is removed we wouldn't know if anybody in\n> the interim _would_ have complained. But it still seems like something\n> that _could_ be useful to an archaeologist in the future.\n\nOK. I do not have problem with \"... apparently some compilers\n(e.g. MS Visual C 2017) complain about ...\".\n\nI just thought MS folks would mind that; if they do want to see the\nname of their compiler in the description, I am fine, too.\n\nThanks.\n"},{"id":"375814","messageId":"nycvar.QRO.7.76.6.1905172051560.46@tvgsbejvaqbjf.bet","threadId":"51083","inReplyTo":"xmqq36lf5e7a.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-17T18:54:02Z","receivedAt":"2019-05-17T18:54:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 16 May 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> >> lacked the \"const\" for that reason, but apparently some compilers\n> >> complain about the parameter type mismatch.\n> >\n> > We could be more explicit, as we know exactly that it is MS Visual C 2017\n> > that is complaining.\n>\n> We could be, but I do not see a point of shaming one particular\n> compiler vendor.\n\nI agree that there is no point in shaming (in fact, there is a lot of harm\nin shaming, everybody who intentionally shames other people needs to have\na look at https://psycnet.apa.org/doiLanding?doi=10.1037%2Femo0000542 to\nfind out what harm they are doing).\n\nThat is why I suggested to be more explicit, not to shame.\n\nOf course, that illustrates that I apparently think a lot differently\nabout this issue: in contrast to you, I do not think that MSVC does a\nparticularly bad thing here, even if it dares disagree with GCC. :-)\n\nCiao,\nDscho\n"}]}