pkt-line: fix incorrect function declaration
cover pkt-line: fix incorrect function declaration
MS Visual C detected a mismatch between the declaration and the definition of set_packet_header(): it is declared with its second parameter missing the const attribute.
It also detected a mismatch between the declaration and the definition of parse_opt_unknown_cb().
These problems must have been introduced very recently; I do not recall seeing them before today in any of Git for Windows' ever-green branches (i.e. master semi-automatically rebased continously onto pu, next, master and maint).
You could not have seen it in git.git's own Azure Pipeline, as Git for Windows' version already has support to build with MSVC (I plan to submit this directly after v2.22.0 is out).
Johannes Schindelin (2):
pkt-line: fix declaration of set_packet_header()
parse-options: adjust parse_opt_unknown_cb()s declared return type
parse-options.h | 4 +++- pkt-line.h | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-)
base-commit: ab15ad1a3b4b04a29415aef8c9afa2f64fc194a2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-192%2Fdscho%2Ffix-set_packet_header-signature-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-192/dscho/fix-set_packet_header-signature-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/192
Signature · 1 line
gitgitgadget
PATCH 1/2 pkt-line: fix declaration of `set_packet_header()`
From: Johannes Schindelin <[email protected]>
When this function was changed in a97d00799a19 (remote-curl: use post_rpc() for protocol v2 also, 2019-02-21) from file-local to global, the declaration was incorrectly missing the const qualifier.
Let's fix that.
--- pkt-line.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
pkt-line.h +1 -1
@@ -25,7 +25,7 @@ void packet_delim(int fd);void packet_write_fmt (int fd,const char *fmt, ...) __attribute__((format (printf,2 ,3 )));void packet_buf_flush (struct strbuf *buf);void packet_buf_delim (struct strbuf *buf); -void set_packet_header (char *buf,int size); +void set_packet_header (char *buf,const int size);void packet_write (int fd_out,const char *buf,size_t size);void packet_buf_write (struct strbuf *buf,const char *fmt, ...) __attribute__((format (printf,2 ,3 )));void packet_buf_write_len (struct strbuf *buf,const char *data,size_t len);
Signature · 1 line
gitgitgadget
Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Johannes Schindelin <[email protected]>
When this function was changed in
a97d00799a19(remote-curl: usepost_rpc()for protocol v2 also, 2019-02-21) from file-local to global, the declaration was incorrectly missing theconstqualifier.
I do not quite get it. Back when the function was file-scope static, it did not even have a separate declaration, and the definition the said commit added looks correct to me.
Having "const int size" parameter in the definition of a function does help the compilers and the developers by making sure any earlier reference to the parameter in the function would not modify it and cause later reference to obtain a different value.
But the parameter treated as a constant without getting modified during the invocation of the function is an implementation detail of the function; there is no point exposing that implementation detail to its callers. It does not even help the compilers handling the caller's compilation unit---the parameter is passed by value, so the caller knows that the callee would not modify it without "const" there.
Does the language even allow flagging "const int in the definition, int in the declaration" as a warning-worthy discrepancy?
Quoted 2 lines
pkt-line.h +1
-1-void set_packet_header (char *buf,int size); +void set_packet_header (char *buf,const int size);
Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`
Hi Junio,
Quoted 5 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 2 lines · writes:
From: Johannes Schindelin <[email protected]>
Quoted 7 lines
Quoted 3 lines
When this function was changed in
a97d00799a19(remote-curl: usepost_rpc()for protocol v2 also, 2019-02-21) from file-local to global, the declaration was incorrectly missing theconstqualifier.I do not quite get it. Back when the function was file-scope static, it did not even have a separate declaration, and the definition the said commit added looks correct to me.
Right, I should have said "the new declaration" instead of "the declaration".
Quoted 13 lines
Having "const int size" parameter in the definition of a function does help the compilers and the developers by making sure any earlier reference to the parameter in the function would not modify it and cause later reference to obtain a different value.
But the parameter treated as a constant without getting modified during the invocation of the function is an implementation detail of the function; there is no point exposing that implementation detail to its callers. It does not even help the compilers handling the caller's compilation unit---the parameter is passed by value, so the caller knows that the callee would not modify it without "const" there.
Quoted 2 lines
Does the language even allow flagging "const int in the definition, int in the declaration" as a warning-worthy discrepancy?
Apparently it does, as MS Visual C does issue a warning (and with /Wall, it fails).
In any case, I don't think that it makes sense to have a function declaration whose signature differs from the definition's.
Ciao,
Dscho
Quoted 3 lines
Quoted 2 lines
pkt-line.h +1
-1-void set_packet_header (char *buf,int size); +void set_packet_header (char *buf,const int size);
Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`
Quoted 8 lines · Johannes Schindelin
Quoted 8 lines
But the parameter treated as a constant without getting modified during the invocation of the function is an implementation detail of the function; there is no point exposing that implementation detail to its callers. It does not even help the compilers handling the caller's compilation unit---the parameter is passed by value, so the caller knows that the callee would not modify it without "const" there.
Quoted 8 lines
Quoted 2 lines
Does the language even allow flagging "const int in the definition, int in the declaration" as a warning-worthy discrepancy?
Apparently it does, as MS Visual C does issue a warning (and with
/Wall, it fails).In any case, I don't think that it makes sense to have a function declaration whose signature differs from the definition's.
I actually agree with Junio's point that in an ideal world the declaration should omit details that are not relevant to the caller. But clearly we do not live in that world, and this is a small enough point that we should fix it in one direction or the other.
I do have a slight preference for going the _other_ way. There is no need to mark the parameter as const in the definition. It is passed by value, so nobody except the function body cares either way. And we have many function bodies where value-passed parameters (or local variables!) are not marked as const, even though they are only assigned to once.
I don't think that annotation is telling much to any reader of the code, nor to a decent optimizing compiler.
-Peff
Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`
Quoted 24 lines · Jeff King
Quoted 8 lines · Johannes Schindelin
Quoted 8 lines
But the parameter treated as a constant without getting modified during the invocation of the function is an implementation detail of the function; there is no point exposing that implementation detail to its callers. It does not even help the compilers handling the caller's compilation unit---the parameter is passed by value, so the caller knows that the callee would not modify it without "const" there.
Quoted 8 lines
Quoted 2 lines
Does the language even allow flagging "const int in the definition, int in the declaration" as a warning-worthy discrepancy?
Apparently it does, as MS Visual C does issue a warning (and with
/Wall, it fails).In any case, I don't think that it makes sense to have a function declaration whose signature differs from the definition's.
I actually agree with Junio's point that in an ideal world the declaration should omit details that are not relevant to the caller. But clearly we do not live in that world, and this is a small enough point that we should fix it in one direction or the other.
Quoted 8 lines
I do have a slight preference for going the _other_ way. There is no need to mark the parameter as const in the definition. It is passed by value, so nobody except the function body cares either way. And we have many function bodies where value-passed parameters (or local variables!) are not marked as const, even though they are only assigned to once.
I don't think that annotation is telling much to any reader of the code, nor to a decent optimizing compiler.
To be clear, I can live with your patch as-is, and I don't think this one site overly matters. Mostly I do not want to see "let's declare pass-by-value parameters as const" picked up as a general concept, and it seems easiest to try to squash the first instance of it. :)
-Peff
Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`
In reply to Jeff KingQuoted 5 lines · Jeff King
I do have a slight preference for going the _other_ way. There is no need to mark the parameter as const in the definition. It is passed by value, so nobody except the function body cares either way. And we have many function bodies where value-passed parameters (or local variables!) are not marked as const, even though they are only assigned to once.
That would be more like this patch, then?
-- >8 --
Subject: pkt-line: drop 'const'-ness of a param to set_packet_header()
The fact that the incoming parameter is used as read-only in the fuction is an implementation detail that the callers should not have to know, and the prototype defined for the function in pkt-line.h lacked the "const" for that reason, but apparently some compilers complain about the parameter type mismatch.
Let's squelch it by removing the "const" that is pointless for a small function like this, which would not help optimizing compilers nor reading humans that much.
--- pkt-line.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
pkt-line.c +1 -1
@@ -119,7 +119,7 @@ void packet_buf_delim(struct strbuf *buf)strbuf_add (buf,"0001" ,4 ); } -void set_packet_header (char *buf,const int size) +void set_packet_header (char *buf,int size) {static char hexchar[] ="0123456789abcdef" ;
Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`
Quoted 8 lines · Junio C Hamano
Quoted 5 lines · Jeff King
I do have a slight preference for going the _other_ way. There is no need to mark the parameter as const in the definition. It is passed by value, so nobody except the function body cares either way. And we have many function bodies where value-passed parameters (or local variables!) are not marked as const, even though they are only assigned to once.
Quoted 5 lines
That would be more like this patch, then?
-- >8 --
Subject: pkt-line: drop 'const'-ness of a param toset_packet_header()[..]
Yes, exactly.
-Peff
Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`
In reply to Junio C HamanoHi Junio,
Quoted 9 lines · Junio C Hamano
Quoted 5 lines · Jeff King
I do have a slight preference for going the _other_ way. There is no need to mark the parameter as const in the definition. It is passed by value, so nobody except the function body cares either way. And we have many function bodies where value-passed parameters (or local variables!) are not marked as const, even though they are only assigned to once.
That would be more like this patch, then?
I can live very well with this patch.
Just two minor comments:
Quoted 8 lines
-- >8 --
Subject: pkt-line: drop 'const'-ness of a param toset_packet_header()The fact that the incoming parameter is used as read-only in the fuction is an implementation detail that the callers should not have to know, and the prototype defined for the function in
pkt-line.hlacked the "const" for that reason, but apparently some compilers complain about the parameter type mismatch.
We could be more explicit, as we know exactly that it is MS Visual C 2017 that is complaining.
Quoted 2 lines
Let's squelch it by removing the "const" that is pointless for a small function like this, which would not help optimizing compilers
It is not pointless because of the size of the function, but because int is already a type that is always passed by value, never by reference.
If at all, it would make a difference if the function was inlined, so I would argue that it would be pointless for large functions.
But both aspects are moot and more philosophical than anything else in this context.
Thanks,
Dscho
Quoted 23 lines
nor reading humans that much.
Noticed-by Johannes Schindelin <[email protected]>Helped-by Jeff King <[email protected]>Signed-off-by Junio C Hamano <[email protected]>--- pkt-line.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)pkt-line.c +1
-1@@ -119,7 +119,7 @@ void packet_buf_delim(struct strbuf *buf)strbuf_add (buf,"0001" ,4 ); } -void set_packet_header (char *buf,const int size) +void set_packet_header (char *buf,int size) {static char hexchar[] ="0123456789abcdef" ;
Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`
Quoted 5 lines · Johannes Schindelin
Quoted 2 lines
lacked the "const" for that reason, but apparently some compilers complain about the parameter type mismatch.
We could be more explicit, as we know exactly that it is MS Visual C 2017 that is complaining.
We could be, but I do not see a point of shaming one particular compiler vendor.
Quoted 5 lines
Quoted 2 lines
Let's squelch it by removing the "const" that is pointless for a small function like this, which would not help optimizing compilers
It is not pointless because of the size of the function, but because
intis already a type that is always passed by value, never by reference.
You are looking only at the prototype side (i.e. declaration in *.h). Yes it is pointless for the callers. For the callee, the story is different and pass-by-value does not even get in the picture.
The mention of pointless-ness I made was about the implementation side (i.e. definition in *.c). For a sufficiently large and complex function implementation, being able to say upfront that this incoming parameter is never modified would help following the logic in the implementation, so "const int param" in the parameter list of a definition is *not* pointless in general. But apparently some compilers are not happy abot "const int param" in implementation that is paired with "int param" in prototype, we are dropping a const that could be useful, with an excuse that for this particular function that is small and trivial, we can live without.
Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`
Quoted 10 lines · Junio C Hamano
Quoted 5 lines · Johannes Schindelin
Quoted 2 lines
lacked the "const" for that reason, but apparently some compilers complain about the parameter type mismatch.
We could be more explicit, as we know exactly that it is MS Visual C 2017 that is complaining.
We could be, but I do not see a point of shaming one particular compiler vendor.
I'd slightly disagree. If 10 years from now we decide that MSVC is not a supported compiler anymore, that information would be useful for somebody digging into the history to say "ah, we can probably use this pattern once more". I say probably because while it was the only complainer, once the pattern is removed we wouldn't know if anybody in the interim _would_ have complained. But it still seems like something that _could_ be useful to an archaeologist in the future.
-Peff
Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`
Quoted 13 lines · Jeff King
Quoted 10 lines · Junio C Hamano
Quoted 5 lines · Johannes Schindelin
Quoted 2 lines
lacked the "const" for that reason, but apparently some compilers complain about the parameter type mismatch.
We could be more explicit, as we know exactly that it is MS Visual C 2017 that is complaining.
We could be, but I do not see a point of shaming one particular compiler vendor.
Quoted 7 lines
I'd slightly disagree. If 10 years from now we decide that MSVC is not a supported compiler anymore, that information would be useful for somebody digging into the history to say "ah, we can probably use this pattern once more". I say probably because while it was the only complainer, once the pattern is removed we wouldn't know if anybody in the interim _would_ have complained. But it still seems like something that _could_ be useful to an archaeologist in the future.
OK. I do not have problem with "... apparently some compilers (e.g. MS Visual C 2017) complain about ...".
I just thought MS folks would mind that; if they do want to see the name of their compiler in the description, I am fine, too.
Thanks.
Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`
In reply to Junio C HamanoHi Junio,
Quoted 10 lines · Junio C Hamano
Quoted 5 lines · Johannes Schindelin
Quoted 2 lines
lacked the "const" for that reason, but apparently some compilers complain about the parameter type mismatch.
We could be more explicit, as we know exactly that it is MS Visual C 2017 that is complaining.
We could be, but I do not see a point of shaming one particular compiler vendor.
I agree that there is no point in shaming (in fact, there is a lot of harm in shaming, everybody who intentionally shames other people needs to have a look at https://psycnet.apa.org/doiLanding?doi=10.1037%2Femo0000542 to find out what harm they are doing).
That is why I suggested to be more explicit, not to shame.
Of course, that illustrates that I apparently think a lot differently about this issue: in contrast to you, I do not think that MSVC does a particularly bad thing here, even if it dares disagree with GCC. :-)
Ciao,
Dscho
PATCH 2/2 parse-options: adjust `parse_opt_unknown_cb()`s declared return type
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
In f41179f16ba2 (parse-options: avoid magic return codes, 2019-01-27), the signature of the low-level parse-opt callback function was changed to return an enum.
And while the implementations were changed, one declaration was left unchanged, still claiming to return int.
This can potentially lead to problems, as compilers are free to choose any integral type for an enum as long as it can represent all declared values.
--- parse-options.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
parse-options.h +3 -1
@@ -286,7 +286,9 @@ int parse_opt_commit(const struct option *, const char *, int);int parse_opt_tertiary (const struct option *,const char *,int );int parse_opt_string_list (const struct option *,const char *,int );int parse_opt_noop_cb (const struct option *,const char *,int ); -int parse_opt_unknown_cb (struct parse_opt_ctx_t *ctx,const struct option *,const char *,int ); +enum parse_opt_result parse_opt_unknown_cb (struct parse_opt_ctx_t *ctx, +const struct option *, +const char *,int );int parse_opt_passthru (const struct option *,const char *,int );int parse_opt_passthru_argv (const struct option *,const char *,int );
Signature · 1 line
gitgitgadget
Re: [PATCH 2/2] parse-options: adjust `parse_opt_unknown_cb()`s declared return type
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 6 lines · writes:
From: Johannes Schindelin <[email protected]>
In
f41179f16ba2(parse-options: avoid magic return codes, 2019-01-27), the signature of the low-level parse-opt callback function was changed to return anenum.
Quoted 6 lines
And while the implementations were changed, one declaration was left unchanged, still claiming to return
int.This can potentially lead to problems, as compilers are free to choose any integral type for an
enumas long as it can represent all declared values.
The enum is meant to represent "these magic negative numbers", and if the compiler chose "long" for it and the implementation of the function returned a "long", while the caller thought it would yield an "int", things will break.
Looks good to make callers' expectation and what callee does consistent.
Quoted 20 lines
Signed-off-by Johannes Schindelin <[email protected]>--- parse-options.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)parse-options.h +3
-1@@ -286,7 +286,9 @@ int parse_opt_commit(const struct option *, const char *, int);int parse_opt_tertiary (const struct option *,const char *,int );int parse_opt_string_list (const struct option *,const char *,int );int parse_opt_noop_cb (const struct option *,const char *,int ); -int parse_opt_unknown_cb (struct parse_opt_ctx_t *ctx,const struct option *,const char *,int ); +enum parse_opt_result parse_opt_unknown_cb (struct parse_opt_ctx_t *ctx, +const struct option *, +const char *,int );int parse_opt_passthru (const struct option *,const char *,int );int parse_opt_passthru_argv (const struct option *,const char *,int );