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

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

[Johannes Schindelin via GitGitGadget]

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

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

[Johannes Schindelin via GitGitGadget]

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(-)

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

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

[Johannes Schindelin via GitGitGadget]

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(-)

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

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

[Junio C Hamano]

"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.

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);

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

[Junio C Hamano]

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

> 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.

>
> 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);

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

[Johannes Schindelin]

Hi Junio,

On Tue, 14 May 2019, Junio C Hamano wrote:

> "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".

> 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);
>

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

[Jeff King]

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.

-Peff

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

[Jeff King]

On Tue, May 14, 2019 at 10:43:06AM -0400, Jeff King wrote:

> 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

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

[Junio C Hamano]

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() 

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(-)

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";
 

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

[Jeff King]

On Wed, May 15, 2019 at 10:42:35AM +0900, Junio C Hamano wrote:

> 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

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

[Johannes Schindelin]

Hi Junio,

On Wed, 15 May 2019, Junio C Hamano wrote:

> 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:

> -- >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

> 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";
>
>

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

[Junio C Hamano]

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.

>> 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.

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

[Jeff King]

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.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Johannes Schindelin]

Hi Junio,

On Thu, 16 May 2019, 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 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