threads / patch / 51083

patch, 2 partspkt-line: fix incorrect function declaration

Subject: [PATCH 0/2] pkt-line: fix incorrect function declaration

## tl;dr

15 messages between May 13, 2019 and May 17, 2019. Diffs are folded; open one to read it.

replies: 14people: 3as markdown or json

Johannes Schindelin via GitGitGadget· May 13, 2019, 22:43 UTC · lore

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
-- 
gitgitgadget
Johannes Schindelin via GitGitGadget· May 13, 2019, 22:43 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 pkt-line.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to pkt-line.h +1 −1
diff --git a/pkt-line.h b/pkt-line.h
index 5c62015db4..04ae7d802c 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -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);

-- 
gitgitgadget
Junio C Hamano· May 13, 2019, 23:24 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

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?

> -void set_packet_header(char *buf, int size);
> +void set_packet_header(char *buf, const int size);
Johannes Schindelin· May 14, 2019, 12:57 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`

Hi Junio,
On Tue, 14 May 2019, Junio C Hamano wrote:
Show 12 quoted lines
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
>
> 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".

Show 15 quoted 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.
>
> 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

> > -void set_packet_header(char *buf, int size);
> > +void set_packet_header(char *buf, const int size);
>
Jeff King· May 14, 2019, 14:43 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`

On Tue, May 14, 2019 at 02:57:01PM +0200, Johannes Schindelin wrote:
Show 16 quoted 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.
> >
> > 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
Jeff King· May 14, 2019, 14:44 UTC · re: Jeff King · lore

Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`

On Tue, May 14, 2019 at 10:43:06AM -0400, Jeff King wrote:
Show 32 quoted lines
> On Tue, May 14, 2019 at 02:57:01PM +0200, Johannes Schindelin wrote:
> 
> > > 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?
> > 
> > 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.

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
Junio C Hamano· May 15, 2019, 01:42 UTC · re: Jeff King · lore

Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`

Jeff King <peff@peff.net> writes:
Show 5 quoted 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.
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.

Noticed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 pkt-line.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to pkt-line.c +1 −1
diff --git a/pkt-line.c b/pkt-line.c
index c9ed780d0b..a0e87b1e81 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -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";
Jeff King· May 15, 2019, 01:44 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`

On Wed, May 15, 2019 at 10:42:35AM +0900, Junio C Hamano wrote:
Show 13 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > 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() 
> [..]
Yes, exactly.
-Peff
Johannes Schindelin· May 15, 2019, 10:39 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`

Hi Junio,
On Wed, 15 May 2019, Junio C Hamano wrote:
Show 9 quoted lines
> Jeff King <peff@peff.net> writes:
>
> > 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:
Show 8 quoted lines
> -- >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.

We could be more explicit, as we know exactly that it is MS Visual C 2017 that is complaining.

> 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

Show 23 quoted lines
> nor reading humans that much.
>
> Noticed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  pkt-line.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index c9ed780d0b..a0e87b1e81 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -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";
>
>
Junio C Hamano· May 16, 2019, 02:24 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 5 quoted 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.

Show 5 quoted 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.

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.

Jeff King· May 16, 2019, 03:42 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`

On Thu, May 16, 2019 at 11:24:25AM +0900, Junio C Hamano wrote:
Show 10 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> 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
Junio C Hamano· May 16, 2019, 04:28 UTC · re: Jeff King · lore

Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`

Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> On Thu, May 16, 2019 at 11:24:25AM +0900, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> >> 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.

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.
Johannes Schindelin· May 17, 2019, 18:54 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] pkt-line: fix declaration of `set_packet_header()`

Hi Junio,
On Thu, 16 May 2019, Junio C Hamano wrote:
Show 10 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> 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

Johannes Schindelin via GitGitGadget· May 13, 2019, 22:43 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 2/2] parse-options: adjust `parse_opt_unknown_cb()`s declared return type

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 parse-options.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to parse-options.h +3 −1
diff --git a/parse-options.h b/parse-options.h
index bd00cf0049..cd756833a9 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -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);
 

-- 
gitgitgadget
Junio C Hamano· May 13, 2019, 23:29 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH 2/2] parse-options: adjust `parse_opt_unknown_cb()`s declared return type

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 12 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

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.

Show 20 quoted lines
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  parse-options.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/parse-options.h b/parse-options.h
> index bd00cf0049..cd756833a9 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -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);

← back to recent threads